Parse File or String input in a uniform way: Is it possible with Generics?

Question

I have a Parser class which creates a collection of String pairs, parsed in the format key=value.

public class Parser{
 private final HashMap<String, String> val;
 private final File source;
 public Parser(final File path){ 
 /*initialise stuff*/
 val=new HashMap<>();
 source=path; //throws exception if null but omitted for brevity
 /* and finally parse the file */
 parse();
 }

parse() reads source line-by-line using Scanner and populates the collection:

public void parse() {
 final Scanner sc=new Scanner(source);
 while(sc.hasNextLine()){
 //do stuff
 }
 sc.close();
}

The problem is, there are other parsers that utilise this class, and sometimes the parser needs to parse a String instead of a File. This leads to duplication of effort. Basically the code snippet above, with its signature changed to public void parse(final String raw). This led me to the following abomination:

private void parseDecision(final String possibleStr){
 final Scanner sc;
 if(possibleStr==null) //use File
 sc=new Scanner(source); //remember source is a field
 else
 sc=new Scanner(possibleStr);
 /*...*/
}

The abomination is being called from both parse (with argument null) and parse(String), and thus avoids code duplication. But when I look at it I can't help thinking there's a better way to do this.

I thought about using Generics. I've never written a Generic class before and I can't coerce Scanner into accepting T as either String or File, which would have been nice.

Which leads me to my question(s):

• Is the use of parseDecision a code smell or am I seeing ghosts?
• Can this be done in a better way?
• If yes, can it be done with Generics?

• Simon Forsberg: Welcome to Code Review! It's a bit hard to review code like this because the code you are showing has been stripped of some context. Unlike Stack Overflow, we prefer to look at real code instead of example code. There is no need to omit stuff for brevity here. Please see the meta question: Why is hypothetical code off-topic for Code Review?
• Marco Acierno: I don't get how can generics could help in this situation. Here you problem is with the "if" that change the possible source, right?
• rath: @MarcoAcierno At the time it seemed like a natural way to go about it, but it seems I was wrong. Using generics to solve this was just a guess, any solution is good :)
• Mathieu Guindon: I have rolled back the last edit - please see What you may and may not do after receiving answers
• rath: @Mat'sMug I was about to ask on chat why. Thank you for the link :)

Answer 1

Yup - that's an abomination.

What you are really looking for is

void parse(Scanner sc) {...}

which can be called from parse(), which passes it the File scanner; and from parse(String), which passes it the string scanner.

It's likely - but not certain - that this method should really be

HashMap<String,String> parse(Scanner sc) {...}

The clue that this might be so is here in your constructor

public Parser(final File path){ 
 /*initialise stuff*/
 val=new HashMap<>();
 source=path; //throws exception if null but omitted for brevity
 /* and finally parse the file */
 parse();
}

You're doing verbing in the constructor, which is a bad sign. It suggests that Parser is (at different times) responsible for parsing the file and acting as a value type. Those are two different concerns, and should have two different representations.

Another bad sign is that you are creating Scanners in the same class that is doing work. Given that in your current interface, you need to create a Scanner, I'm inclined to guess that the refactoring you need is to create a new class that provides the parse(Scanner) implementation. The existing Parser delegates the work while providing the backward compatible behavior.

If the existing Parser needs more than just key/value pairs, then you might really be looking for

ParseResult parse(Scanner sc) {...}

where ParseResult contains the Map and whatever other state the application needs after the parsing is completed.

• rath: Accepted solution because it requires minimum investment of time (compared to h.j.k's below). This is so simple I feel rather silly for not seeing it before. Anyway, the Parser mostly extends Iterator for loops over the Hashmap anyway, so I've made all parsing methods static, pointing to the beautiful static HashMap parse(Scanner). And now my class shedded a bunch of lines (in hackerish side-methods) as if by magic. Thank you

Answer 2

Here's a different suggestion, would it be better for you to rely on java.util.Properties to do your parsing? If so, the class's load() method accepts a Reader implementation, which you can supply with either a StringReader or FileReader (or in my case, I prefer wrapping my FileReader in a BufferedReader). Something below is workable Java code, and you can start off from there...

public static void main(String[] args) throws IOException {
 Properties props = new Properties();
 props.load(new StringReader("abc=123"));
 System.out.println(props.get("abc"));
 props.clear();
 try (final Reader reader = new BufferedReader(new FileReader("/path/to/file"))) {
 props.load(reader);
 }
}

edit: As you have rightly pointed out/discovered, Scanner is not a generified (?) class, so the closest approximation (in terms of reading code) is to use instanceof checks and then call the appropriate Scanner constructor. Some people frown on it, but the option exists...

• rath: Wow, Properties do exactly what I was doing, even down to comment support. Thank you for this
• h.j.k.: @rath, glad to help!