Monday, February 15, 2016

Refactoring code: Factory Pattern With Generics

Most of us can look at the following example and recognize that it could be written much more elegantly. But lets look into exactly why that is the case and how we can fix it.


The first clue that something is wrong is that IntelliJ highlights both the lines parser.doSomething() in yellow with the warning:
Unchecked assignment: 'java.util.List' to 'java.util.List'. Reason: 'parser' has raw type, so result of parse is erased.
So what does this mean?  Java does type checking at compile time. Since parser was declared with a  raw type, the Java compiler has no way to know the type of the List returned by parse()

But whats all the fuss about? Is this just about removing annoying yellow lines and compiler warnings? Well its a bit deeper than that, since the warning is actually pointing to a flaw in the design.

The reason why the developer used the raw type was because she wants to use the same parser to parse both InvoiceLine and InvoicePaymentLine types. If you actually add a type to the Parser, the warning will go away for one of parse calls but have a compilation error for the other.

One way (not the only way) to fix this is to have a common type for both InvoiceLine and InvoicePaymentLine. Lets make a common interface called Invoiceable. That way we can declare the parser as Parser parser.

That combined with changing the Lists to the same type and a single parse method will get rid of the initial warnings that we saw:

Furthermore the two loops are essentially doing the same thing. Instead of repeating them. Lets just iterate over our List of Invoiceables and set the invoice. To do this we have to add a setInvoice function on the interface. I also removed the check to isEmpty(). Its good practice to not return null and just an empty List making empty and null checks unecessary and cleaning up the verbosity of the code.

And finally, all of the if-then statements are just figuring out how to create a parser. Abstracting out the creation of objects is a good candidate for a Factory method.
The result of main looks like this.

And Beyond 


It looks much cleaner than before and the logic is much easier to follow. As with any refactoring it depends on your situation on what is good enough for the moment and how you think the code may change. You can always add more layers of abstraction or determine that when you see more use cases.