-1
\$\begingroup\$

In the main method, I have to call a factory to retrieve the Parser and cast it to a child parser to read the different file.

How should I improve the code for it to make more sense? Can I avoid casting the parser?

public interface Parser<T>{
  public Parser read(T t){};
}

public class ParserFromJson implements Parser<JSON>{
  @Override
  public Data read(JSON json){...};
}

public class ParserFromXml implements Parser<XML>{
  @Override
  public Data read(XML xml){...};
}

public class ParserFactory{
  public Parser create(Type type){
     switch(type){
        case JSON:
            return new ParserFromJson();
        case XML:
            return new ParserFromXml();
  }
}

public enum Type{
    JSON, XML
}

public static void main(String[] args){
   ((ParserFromJson)ParserFactory.create(JSON)).read(jsonfile);
   ((ParserFromXml )ParserFactory.create(XML)).read(xmlfile);
}

=========================================================================== Update

I'm currently use @Imus Option 1 as the solution because I feel the Factory is redundant. Plus factory can not return concrete class but parent class.

However, I'm still have a feel that this may not be the superior result. Because I'm not sure if it breaks the OOP principle to use concrete too much.

I want to understand if my final OOP design is making sense: The code should able to parse JSON, XML(no control on this two object) and convert the content to an object Data (I'm able to define the structure). Now consider if there is a new type of input format coming in, Let's say HTML, then I need to create a new impl class ParserFromHtml, add a new enum in Type.

If I keep the ParserFactory, then I also need to add a new switch case plus cast using it later on. And I've learned that good OOP design should minimal the code you need to change for the existing class.

Therefore, I'm questioning myself if the design make sense or not? Or actually there is a more advanced pattern to handle this scenario. I personally like my idea to use Generic and have different concrete class to parse different input accordingly. But What if this fundamental thinking is wrong?

\$\endgroup\$

closed as off-topic by 200_success, Heslacher, IEatBagels, Graipher, Sᴀᴍ Onᴇᴌᴀ Jan 30 at 18:55

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – 200_success, IEatBagels, Graipher, Sᴀᴍ Onᴇᴌᴀ
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ Hi, your code looks more like some stub code to illustrate a situation you have. In order to provide correct review, we need the fully working code. Please add the real implementation of this code and the review will only be better :) \$\endgroup\$ – IEatBagels Jan 30 at 13:31
  • \$\begingroup\$ Hello @IEatBagels, thank you for your feedback. I can't provide concrete code for some reason. But my extraction should be able to illustrate the problem. If you think this is not appropriate, should I move this to a different forum? Or maybe I can try to add more description to concrete my question? \$\endgroup\$ – Zingoer Jan 31 at 14:54
  • \$\begingroup\$ @IEatBagels If you mean that the question is missing the code-context for how ParserFromJson or ParserFromXml actually parses the XML/JSON, then I consider that highly irrelevant. \$\endgroup\$ – Simon Forsberg Feb 1 at 11:20
  • \$\begingroup\$ @Zingoer What could possibly be interesting is the following: What is Data? What is jsonFile/xmlFile? Additionally, your interface does not currently compile with that code. Also, you are saying that you have no control of the XML/JSON objects, but that you would have the possibility to add HTML in the enum which confuses me as XML and JSON are values of the enum - or are there other objects? \$\endgroup\$ – Simon Forsberg Feb 1 at 11:24
  • \$\begingroup\$ @SimonForsberg I was mostly referring to how the parsers will be used. It's hard to tell if this implementation is solid without knowing how it's used. I probably should have included that in my original comment. \$\endgroup\$ – IEatBagels Feb 1 at 15:12
2
\$\begingroup\$

Depending on how exactly you define your "files" there's 2 main options here.
1) Keep parsers entirely split without trying to force them into the same interface
2) Use a common file interface to pass onto both parsers

Option 1 is probably the simplest one. Just implement the factory as follows:

public class ParserFactory{
    public ParserFromJson creatJSONParser(){
        return new ParserFromJson();
    }
    public ParserFromXML creatXMLParser(){
        return new ParserFromXml();
    }
}

Or since you know beforehand which class you want, just instanciate that without a factory.


The other option is to modify the separate parsers to just take a more general File instead of your specific JSON / XML file formats.

public interface Parser {
  public Parser read(File t){};
}

public class ParserFromJson implements Parser {
  @Override
  public Data read(File jsonFile){...};
}

That way you don't need the cast anymore:

Data output = ParserFactory.create(JSON).read(jsonfile);

That last option still requires you to first figure out which type you need to pass to the factory. You could try to go a step further and have the factory decide which parser to use for a specific file.

public class ParserFactory{
    public Parser create(File file){
        if(file.getName().endsWith(".xml"){
            return new ParserFromJson();
        } 
        if (file.getName().endsWith(".json"){
            return new ParserFromXml();
        }
        throw new UnknowFileFormatException("No parser known to handle {}",filegetName());
     }
}

That way you can just call it as follows:

Data output = ParserFactory.create(jsonfile).read(jsonfile);

You could also store the file directly in the Parser when you create it so you can call it as ParserFactory.create(jsonfile).read(); instead but I'm already feeling we're overdesigning everything for this simple example.

Check what it's going to be used for and decide how much effort you should put in generalising these parsers. Sometimes option 1 is all you need to get things done so you can put time and effort in more important tasks instead.

\$\endgroup\$
  • \$\begingroup\$ It would make sense to re-use a parser, so storing the file reference directly in the parser doesn't sound like a good idea. Overall a nice answer! \$\endgroup\$ – Simon Forsberg Jan 29 at 15:53
  • \$\begingroup\$ @SimonForsberg That too highly depends on how you would use it. I see the parser as a small object that you create just to parse the file and then throw away so it's fine to create a new one for each file. Especially so if you would parse it in small chunks to handle instead of all at once. If a parser is expensive to create you would be correct to only create one and reuse that for each file. \$\endgroup\$ – Imus Jan 30 at 7:57

Not the answer you're looking for? Browse other questions tagged or ask your own question.