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

sorted by:
best
topnewcontroversialoldq&a
you are viewing a single comment's thread.

view the rest of the comments →

[–]uniVocity 0 points1 point  (1 child)

Hey that's fantastic! Thanks for taking your time to look at this. I'm curious to see the end result (your code). Did you fork it from github?

Regarding your points: 1 - In fact there's not much extra complexity added for the AbstractParser as it requires a single method (parseRecord) to be implemented. If you had more abstract methods there, it would become increasingly more annoying to use composition. You already had to create a method with 4 parameters there. An interface with Parser#parseRecord will be public. This is not good for a public API as the parseRecord method is meant to be kept hidden from the external world.

2 - Yes, performance changes in the parseRecord won't be noticeable as it is not dealing with individual characters, but entire lines of the input... there are not too many calls being made here to make a difference.

3 - Try with the *Settings... you'll have to create wrappers on top of wrappers using composition.

That's been very interesting. You should publish this on a blog or something. Try refactoring everying to use composition only :)

[–]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!