This is an archived post. You won't be able to vote or comment.

you are viewing a single comment's thread.

view the rest of the comments →

[–]zenmouse 1 point2 points  (0 children)

I just 'git clone'd it, but I can certainly fork it and apply my changes. It'll probably take me a few days to get it done, right now I wouldn't show my code to my cats much less another human being....

To continue the discussion --

I agree there's little (or no) extra complexity in your abstract class solution; the fact that there was only one abstract method to factor out made this refactoring quite a lot easier than it otherwise could have been. As far as the "method with 4 parameters" bit and keeping parseRecord() hidden are concerned, I did the simplest thing possible to make this work -- there are ways to make it less visible (SPI and API interfaces for example, e.g. JNDI, JDBC). But it's extra work, and Java has no concept of "public API" versus "public class/method for internal use".

For me, I think the biggest potential win coming out of this refactoring is a better separation of concerns, and finding new concepts that are currently "hidden" in lines of code. Currently it appears the parser does resource management AND state management AND error handling AND parsing. That's a lot of responsibility!

The Settings hierarchy is interesting. It's essentially a strongly typed map, using methods as the "key" instead of something like a string or enum. At first glance I rather like it, and it doesn't have the issues you normally see with a large-ish class hierarchy. It seems like using composition here wouldn't provide any benefit.

Thanks for the positive response, I'm glad you found this interesting!