public static String findsite(InputStream text) {
String site, line = null;
Boolean nospace = true;
BufferedReader reader = new BufferedReader(new InputStreamReader(text));
try {
line = reader.readLine();
} catch(Exception e) {}
if(line.indexOf("Full") == 0) {
site = "fulltilt";
} else if((line.indexOf("Pok") == 0) || (line.indexOf("POK") == 0)) {
site = "pokerstars";
} else if(line.indexOf("***** Hand History") == 0) {
site = "888";
} else if(line.indexOf("***") == 0) {
site = "partypoker";
} else if(line.indexOf("** Game ID") == 0) {
site = "prima";
} else if(line.indexOf("Everest") == 0) {
site = "everest";
} else if(line.indexOf("Betfair") == 0) {
site = "betfair";
} else if((line.indexOf("STAGE") == 0) || (line.indexOf("Stage") == 0)){
site = "cereus";
} else if((line.indexOf("Game #") == 0) || (line.indexOf("GAME #") == 0)) {
site = "ipoker";
} else if(line.indexOf("** Hand #") == 0) {
site = "ladbrokes";
} else if(line.indexOf("WPEX") == 0) {
site = "wpex";
} else if(line.indexOf("Hand Start.") == 0) {
site = "tribeca";
} else if(line.indexOf("<game id") == 0) {
site = "carbon";
} else if(line.indexOf("Bodog") == 0) {
site = "bodog";
} else if(line.indexOf("Hand") == 0) {
site = "cake";
} else {
site = "fulltilt";
}
return site;
}
public void txtToHand(InputStream sin) {
String site;
String tourn = "false";
//remove any blanklines and such
sin = striphistory(sin);
sin.mark(0);
site = findsite(sin);
try {
sin.reset();
} catch(Exception e) {}
try {
// Create an input character stream from standard in
ANTLRInputStream input = new ANTLRInputStream(sin);
if( site == "fulltilt") {
FullTiltLexer lexer = new FullTiltLexer(input);
CommonTokenStream tokens = new CommonTokenStream(lexer);
FullTiltParser parser = new FullTiltParser(tokens);
parser.hand_history();
if(parser.hd.seats.isEmpty()) {
throw new NoSeatsException();
}
parser.hd.site = "fulltilt";
parser.hd.site_id = 1;
hand = parser.hd;
} else if (site == "pokerstars") {
PokerStarsLexer lexer = new PokerStarsLexer(input);
CommonTokenStream tokens = new CommonTokenStream(lexer);
PokerStarsParser parser = new PokerStarsParser(tokens);
parser.hand_history();
parser.hd.site = "pokerstars";
parser.hd.site_id = 2;
hand = parser.hd;
} else if (site == "partypoker") {
PartyPokerLexer lexer = new PartyPokerLexer(input);
CommonTokenStream tokens = new CommonTokenStream(lexer);
PartyPokerParser parser = new PartyPokerParser(tokens);
parser.hand_history();
parser.hd.site = "partypoker";
parser.hd.site_id = 3;
hand = parser.hd;
} else if (site == "cereus") {
CereusLexer lexer = new CereusLexer(input);
CommonTokenStream tokens = new CommonTokenStream(lexer);
CereusParser parser = new CereusParser(tokens);
parser.hand_history();
parser.hd.site = "cereus";
parser.hd.site_id = 4;
hand = parser.hd;
} else if (site == "cake") {
CakeLexer lexer = new CakeLexer(input);
CommonTokenStream tokens = new CommonTokenStream(lexer);
CakeParser parser = new CakeParser(tokens);
parser.hand_history();
parser.hd.site = "cake";
parser.hd.site_id = 5;
hand = parser.hd;
} else if (site == "prima") {
PrimaLexer lexer = new PrimaLexer(input);
CommonTokenStream tokens = new CommonTokenStream(lexer);
PrimaParser parser = new PrimaParser(tokens);
parser.hand_history();
parser.hd.site = "prima";
parser.hd.site_id = 6;
hand = parser.hd;
} else if (site == "ipoker") {
IPokerLexer lexer = new IPokerLexer(input);
CommonTokenStream tokens = new CommonTokenStream(lexer);
IPokerParser parser = new IPokerParser(tokens);
parser.hand_history();
parser.hd.site = "ipoker";
parser.hd.site_id = 7;
hand = parser.hd;
} else if (site == "888") {
EightLexer lexer = new EightLexer(input);
CommonTokenStream tokens = new CommonTokenStream(lexer);
EightParser parser = new EightParser(tokens);
parser.hand_history();
parser.hd.site = "888";
parser.hd.site_id = 7;
hand = parser.hd;
}
} catch(Exception e) {}
}
Refactorings
No refactoring yet !
bob
May 21, 2009, May 21, 2009 18:42, permalink
First of all, you should not use "==" to compare String references. == compares if the references point to the same object, not if the String contents are equal. You should use the ".equals()" method instead. It may work in this case because the strings you are comparing were all String literals, and String literals are automatically interned, but you shouldn't count on it.
Second, it seems that you have a lot of repeated code. It may be better to create an array or collection of "entries" objects, each of which contains information about the string to search for, the site name, and the ID; and then loop through the array to look for the right one; that way the processing code is only in one place in the loop.
Also, "line.indexOf(blah) == 0" should be "line.startsWith(blah)".
Stacy Curl
May 25, 2009, May 25, 2009 03:19, permalink
I tend to operate under the faith (justified by some experience) that I can improve the performance of clear code more easily than clarify the expression of performant code; thus sticking to the tenets of my religion I've refactored for clarity !
I've taken all the if-else cases and built a data driven BruteForceSearcher that operates essentially the same way, most like not an improvement in performance. I did a bit of searching on wikipedia and found that a RabinKarpSearcher might be what you are looking for, of course the gain may be slight and the added complexity not worth it (I do think the clarity is worth it, tho') (http://en.wikipedia.org/wiki/Rabin-Karp_string_search_algorithm).
The next thing I've done is banish the use of a String (*) to represent a site, instead I've used ... a Site ! I've moved the lexing / parsing behaviour onto Site greatly shortening txtToHand, there's still some procedural code in txtToHand (the NoSeatsException bit for example) which could be moved to a FullTiltSite class perhaps.
(*) I hate returning "other people's types" because by definition they have nothing to do with my own domains, I can't add any behavour onto them and am reduced to treating them like C structs, I'll even go as far as to write trival wrappers with no behaviour because at least they serve as a placeholder and can easily be removed, whereas tracking down which Strings in a code base mean a Site and which mean a PostCode is hard graft. Rant over :p
Best Regards,
Stacy.
public static Site findsite(InputStream text) {
String line = null;
Boolean nospace = true;
BufferedReader reader = new BufferedReader(new InputStreamReader(text));
try {
line = reader.readLine();
} catch(Exception e) {}
return buildSearcher().search(line, fulltilt);
}
private static Searcher buildSearcher() {
return new BruteForceSearcher<Site>()
.addTerm("Full", pokerstars)
.addTerm("Pok", pokerstars)
.addTerm("POK", pokerstars)
.addTerm("***** Hand History", 888)
.addTerm("***", partypoker)
.addTerm("** Game ID", prima)
.addTerm("Everest", everest)
.addTerm("Betfair", betfair)
.addTerm("STAGE", cereus)
.addTerm("Stage", cereus)
.addTerm("Game #", ipoker)
.addTerm("GAME #", ipoker)
.addTerm("** Hand #", ladbrokes)
.addTerm("WPEX" wpex)
.addTerm("Hand Start." tribeca)
.addTerm("<game id", carbon)
.addTerm("Bodog", bodog)
.addTerm("Hand", cake);
}
public interface Searcher<Result> {
String search(String text, Result defaultResult);
}
// Probably slower than if-else-if...
public static BruteForceSearcher<Result> implement Searcher<Result> {
private final SortedMap<String, Result> searchTerms =
new TreeMap<String, Result>();
public BruteForceSearcher addTerm(String term, Result result) {
searchTerms.put(term, result);
return this;
}
public Result search(String text, Result defaultResult) {
for (Entry<String, Result> term : searchTerms) {
if (text.startsWith(term.key()) {
return term.value();
}
}
return defaultResult;
}
}
private final Site fulltilt = new ReflectiveSite("fulltilt", 1,
FullTiltLexer.class, FullTiltParser.class);
private final Site pokerstars = new Site("pokerstars", 2,
PokerStarsLexer.class, PokerStarsParser.class);
private final Site partypoker = new Site("partypoker", 3,
PartyPokerLexer.class, PartyPokerParser.class);
private final Site site888 = new Site("888", 8,
EightLexer.class, EightParser.class);
private final Site prima = new Site("prima", 6,
PrimaLexer.class, PrimaParser.class);
private final Site everest = new Site("everest", ?);
private final Site betfair = new Site("betfair", ?);
private final Site cereus = new Site("cereus", 4,
CereusLexer.class, CereusParser.class);
private final Site ipoker = new Site("ipoker", 7,
IPokerLexer.class, IPokerParser.class);
private final Site ladbrokes = new Site("ladbrokes", ?);
private final Site wpex = new Site("wpex", ?);
private final Site tribeca = new Site("tribeca", ?);
private final Site carbon = new ReflectiveSite("carbon", ?);
private final Site bodog = new ReflectiveSite("bodog", ?);
private final Site cake = new ReflectiveSite("cake", 5,
CakeLexer.class, CakeParser.class);
public interface Site {
Parser parser(String input);
boolean isOneOf(Site ... sites);
}
// I've chosen to use generics + reflection here, rather than create a parallel
// heirarchy of factories.
public static class ReflectiveSite implements Site {
public Site(String name, long id,
Class<? extends Lexer> lexerClass,
Class<? extends Parser> parserClass) {
...
}
public boolean isOneOf(Site ... sites) {
return new HashSet<Site>(Arrays.asList(sites)).contains(this);
}
public Parser parse(String input) {
return parserClass.newInstance(
new CommonTokenStream(lexerClass.newInstance(input)));
}
}
public void txtToHand(InputStream sin) {
String tourn = "false";
//remove any blanklines and such
sin = striphistory(sin);
sin.mark(0);
Site site = findsite(sin);
try {
sin.reset();
} catch(Exception e) {}
try {
// Create an input character stream from standard in
ANTLRInputStream input = new ANTLRInputStream(sin);
Parser parser = site.parse(input);
if (site.isOneOf(fulltilt, pokerstars, partypoker, cereus,
cake, prima, ipoker, site888)) {
parser.hand_history();
if(site == fulltilt && parser.hd.seats.isEmpty()) {
throw new NoSeatsException();
}
parser.hd.site = site.name();
parser.hd.site_id = site.id();
hand = parser.hd;
}
} catch(Exception e) {}
}
feydr.myopenid.com
May 26, 2009, May 26, 2009 16:06, permalink
I do like the use of reflection here -- that for sure I'll prob drop into the new code; as for returning strings -- you are totally right; that's about the first thing someone brings up about this code and I totally believe that as well...
Andrew Binstock
August 25, 2009, August 25, 2009 03:28, permalink
Learn and use the Command pattern. It was designed for precisely the problem you initially are trying to clean up.
I don't know too much about java but this problem is not really language specific -- it just looks horrible..
Basically we have 6-8 grammars that get loaded depending on what the first line or so of a file looks like.
So not only do we match that first line but then we have to come back through and match the site name just
to load 1 of 8 different grammars.
There are two functions here (hope I didn't cut anything important out).
findsite basically determines from the first line what grammar we are going to deal with..
txtToHand actually loads our grammar
-- perhaps you can make it cleaner, more succint and even speed it up? -- speed is really the most important feature I want in this refactor