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?

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.

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...