I have inherited a project to which I'm adding some new functionality, and I have a pretty basic design question:
The project has - among other things - two packages that encapsulate code for adapters to vendor-specific implementations of some business logic.
Just to get this out in front: In the initial version of the application, there was only one vendor. When the second vendor came on line, there was no real attempt to refactor out common functionality. This is the situation I have inherited.
Now I'm adding a third vendor, and I AM trying to do some refactoring - at least the most obvious and least dangerous things.
So, here's the first of probably several questions:
Each package has a version of a class that contains three methods, two of which are identical - I mean copy and paste identical. My current solution:
I created a new package, parallel to the two existing ones and am calling it abstractadapter. I put an abstract class in this package that contains the two methods and now have the three concrete classes (existing two ones and a new one for the new vendor) extending this class.
Is this an OK way to go about doing this? Or would it be better to have a utility class? Is this a recognizable design pattern (I'm guessing yes) and, if so, which one? Can someone recommend a good design book for beginners?
p.s. looking forward through the code, this is just an example of many such places where code has been duplicated. So, whatever the solution is, I'll probably be using it in several places.
It's hard to give a detailed answer based on a general description like that. The one thing I would question is your decision to create another package called abstractadapter. IMO, that's a bit pointless, as packages and package names go. I can imagine a package named adapter or adapters. Then a class, AbstractAdapter, in the adapters package. I would probably still not be satisfied with a package named "adapters" and would explore the domain model to see if there is a concept that suggests a course-grained "bucket" concept under which adapters should be organized. On the other hand, adapters are usually used to solve architectural and design issues and not really something that represents a business domain concept, so "adapters" might be as good a package name as you can get to describe its intent well. I'd need to have more context to comment more.
Sorry, I was just at the gym and realized that I left out some important info:
The three classes (two old ones and one new one) are all MDBs. The MDBs are all connected to the same queue. There is a property in the queue's messages that specifies for which vendor the message is intended, thus selecting the appropriate MDB. Other than the onMessage() method, there is nothing in these three MDBs except for the two methods I want to extract out. For the sake of privacy, I'll call the vendors VendorA, VendorB and VendorC. All of the code that is required to process and reroute messages (via web services) to VendorA are in a package called vendoraadapter, all code for VendorB is in vendorbadapter, etc. These packages have a lot of subpackages. I am thinking about extracting all generic code for these packages to a package called abstractadapter. Maybe baseadapter would be better? Or maybe it's just a bad idea altogether?
In the case of the MDBs, baseadapter would have a subpackage called listener (which is the subpackage the two existing MDBs are in), and it would contain an abstract class with the common functionality for the three vendor-specific MDBs. Like I said, I would probably have to repeat this for several other classes and packages, since there is A LOT of duplicate code. There are constraints, however, since we have only limited resources to do any refactoring - and especially retesting - of the existing code. Another aspect: looks like there's a VendorD and VendorE in the relatively near future.
So just to confirm. These vendor modules provide different implementations of the same domain function in your software? In the same way that Hibernate, JdbcTemplate, and h2 all implement persistent storage?
I'm going to assume the answer is yes for now.
Whether your approach will work out for you depends on what those two methods do that are identical between implementations. If they are performing a function that is always going to be the same no matter how many vendors you add in the future then I would guess that the code really has nothing to do with actually adapting your software to the vendor module. Perhaps they don't belong in the adaptor class at all? If it were me then I would start by extracting an interface from your vendor specific classes so that you decouple the rest of your code from having to deal with the vendor specific implementations.
The next thing you've noticed is that methodOne() and methodTwo() are identical across all implementations. Have a think about what the methods are doing. If they are doing common setup and/or teardown type operations that are not specific to each vendor then your current approach of pushing them up into an abstract class is a reasonable way to go. If they are not related to the vendor or domain functionality then you may want to consider if they belong in there at all. You know your code so you'll have to make the call on that one. Lets say you think the abstract class will work for you.
In my experience, it seems fairly common for maintenance developers to want to follow the established "pattern" for organizing things and that's usually a good thing, if the "pattern" makes sense. However, an established pattern should be challenged if there is reason to challenge it. IMO, names like vendoraadapter and vendorbadapter suggest implementation classes rather than packages. If I were to organize this, I would probably do something like this instead:
From the additional information you provided, I'm not even sure that "adapter" is the right word to use for these things. Since the only difference, as you say, is in the onMessage() method, I detect a code design smell there. In particular, the orthogonal responsibilities of the onMessage() method and the other two replicated methods should probably be separated into two different classes. That is, the appropriate refactoring may be to split the responsibility of handling and routing the message (onMessage()) from the responsibility fulfilled by the two other methods. One class is responsible for handling and dispatching messages to another class that has the responsibility encoded in the two replicated methods. This does not sound anything like the Adapter pattern to me.