Problem
I’m fairly new to real world Java so I tried to rewrite an old app I wrote in vanilla java. I’ve made this one a lot more OO and used a dependency/build manager (Gradle). Now I just want to see what more experienced devs have to say about the project.
This is the main code snippet that does most of the work. Any comments here are welcomed.
src/pw/jor/imgurwallpaper/gui/Worker.java : 37
// get url
selection= getGUISelection(selectedURLIndex++);
url = createURL(selection);
// get parser contents
body = Downloader.getPageContents(url);
// parse body for image hashes
ParserAbstract parser = getParser(selection);
parser.parse(body);
// write hashes to file
Writer.writeFiles(parser.getImageHashes());
Solution
You are very very very procedural here. Java does allow this to some extent, but it feels rather clunky. Be aware that there’s significant lack of context here, so I will have to make a few well-educated guesses in reviewing this code…
selection = getGUISelection(selectedURLIndex++);
there’s a few things wrong with this: The first thing for you to learn will be “Tell, don’t ask”.
Wherever this code is, you should be passing it the GUISelection through an ActionListener or similar by invoking the method with that already done. selection should be a parameter.
Actually… scratch that:
url = createURL(selection);
this should be the paraemter to the method we’re in right now.
You need to get your responsibilities straight: one method does one thing. and it does that well…
Work on objects, avoid the static context if you can:
body = Downloader.getPageContents(url);
this should be using an instance:
body = downloader.getPageContents(url);
Did I already mention you should declare variables as close as possible to their usage?
This looks like you fell prey to ages old convention to declare your variables at the top of your procedure. This makes code overly complex to follow for longer procedures and it’s plain confusing.
String body = downloader.getPageContents(url);
would be better.
ParserAbstract parser = getParser(selection);
again: Tell, don’t ask. Additionally: work on objects. Creating a specific Parser for a selection is a separate responsibility. As much as I hate them, this is a use-case for a Factory:
ParserAbstract parser = ParserFactory.getParser(selection);
In addition to that, ParserAbstract sounds like an abstract class. I daresay it’s probably better off as an interface instead, and if not: at least put the Abstract before the Parser. But I think the following is more idiomatic java:
Parser parser = ParserFactory.getOrCreateParser(selection);
parser.parse(body);
// write hashes to file
Writer.writeFiles(parser.getImageHashes());
this seems like an anti-pattern to me. A parser usually is not stateful, iow. it should not keep the results of parsing somewhere, but just return them.
Also again: Tell, don’t ask, and again you work on a Class instead of an instance with the Writer.
more idiomatic for java would be something like:
writer.writeFiles(parser.parse(body));
or a little more stretched out and explicit:
ParseResults imageHashes = parser.parse(body);
writer.writeFiles(imageHashes);