Java Forum / General / November 2006
Design question - implentation and composition
Lionel - 10 Nov 2006 00:08 GMT Hmmm, the word for implementing an interface has slipped my mind, anyway.
I have a design where it seems appropriate to use a tactic I haven't seen before.
I've got four classes each of which subclass a common super class. There are four interfaces which are divided into two groups. Each of the sub classes must implement one interface from each group. However this has meant that I have had some code duplication. This duplication is only simple, a couple of setters and getters, at most any method is 4 lines long.
However, it seems appropriate to reuse the code because it /is/ identical and always should be. So the thought comes to mind that I implement each of the interfaces with a separate class. I then contain an instance of the two required implementations in each of the four subclasses, while also still implement the two interfaces and the methods just become wrappers around the instances of the implementations of the interfaces.
The question: Is this a strange design? Is there a better way to do it?
In fact the design is more complicated, it implements the abstract factory pattern providing two factory methods. There is also a subclass of each of the four classes which provides additional functionality again, each subclass implementing a new interface.
Thanks
Lionel.
Daniel Pitts - 10 Nov 2006 00:43 GMT > Hmmm, the word for implementing an interface has slipped my mind, anyway. > [quoted text clipped - 26 lines] > > Lionel. I might first try to simplify by reducing the number of interface needed. Then, once I've done that, re-evaluate the heirarchy. Its hard to tell you if the design is good without more details about the interfaces/classes.
Lionel - 10 Nov 2006 00:52 GMT >> Hmmm, the word for implementing an interface has slipped my mind, anyway. >> [quoted text clipped - 29 lines] > I might first try to simplify by reducing the number of interface > needed. The interfaces are required in my opinion. There is a lot of common functionality in the super class, the majority of the code resides here, however, each subclass must implement these interfaces, or somehow contain the methods that are in these interfaces. I don't see how there is a way around that.
Then, once I've done that, re-evaluate the heirarchy.
I've considered the heirarchy a number of times and considered what other options I could use. I keep seeing the current heirarchy as the best.
Its hard
> to tell you if the design is good without more details about the > interfaces/classes. Understandably. It is hard to give further detail without information overload. I might think about posting the UML class diagrams somewhere.
Thanks
Lionel.
Daniel Pitts - 10 Nov 2006 01:04 GMT > >> Hmmm, the word for implementing an interface has slipped my mind, anyway. > >> [quoted text clipped - 51 lines] > > Lionel. Or, give simplified examples. Is there a reason that the interfaces can't be combined? Is there a reason that the base class can't implement the interfaces? etc... and so forth.
Lionel - 12 Nov 2006 22:37 GMT > Or, give simplified examples. I'll work on a simplified example.
> Is there a reason that the interfaces can't be combined? Briefly, if we have interfaces InterfaceA1, InterfaceB1, InterfaceA2 and InterfaceB2.
We must have four classes as follows:
Class1 implements InterfaceA1, InterfaceB1
Class2 implements InterfaceA1, InterfaceB2
Class3 implements InterfaceA2, InterfaceB1
Class4 implements InterfaceA2, InterfaceB2
Each class produces quite different results (drug dose optimisation).
Is there a
> reason that the base class can't implement the interfaces? etc... and > so forth. There are several methods in the base class that are common to all subclasses. A lot are implemented in the base class and there are some abstract ones that the base class depends on implementation in subclass.
This probably doesn't clarify too much so I will work on an example and post back in a new thread in the near future.
Thanks for you help :).
Lionel.
Chris Uppal - 10 Nov 2006 11:49 GMT > Hmmm, the word for implementing an interface has slipped my mind, anyway. "Implement" ;-)
> I've got four classes each of which subclass a common super class. There > are four interfaces which are divided into two groups. Each of the sub > classes must implement one interface from each group. My knee-jerk reaction is that this is probably over-complicated. But then, I don't know what forces led you to this design.
> However this has > meant that I have had some code duplication. This duplication is only > simple, a couple of setters and getters, at most any method is 4 lines > long. Getters and setters in an interface ? Indeed, getters and setters are highly suspect in any OO design.
> However, it seems appropriate to reuse the code because it /is/ > identical and always should be. A reasonable reason ;-)
> So the thought comes to mind that I > implement each of the interfaces with a separate class. I then contain > an instance of the two required implementations in each of the four > subclasses, while also still implement the two interfaces and the > methods just become wrappers around the instances of the implementations > of the interfaces. I think the important point here is that -- in a sense -- it doesn't matter. As long as these details are not externally visible (which includes: not constraining the design of the super- or sub-classes) then you can implement those "duplicated" methods in any way you want. If it feels right to pull the duplication out into a separate class, then go for it. If it feels like over-engineering then don't.
BTW, another option (which may make no sense at all in your context) would be to put the shared methods (with 'protected' visibility) into the common superclass, and then re-expose them only in the subclasses where they make sense. Of course, that means that the super-class is "aware" of its subclasses in the sense that it has been co-designed with them, and has features that are specifically intended for those subclasses. But I don't think there is anything wrong with such clusters of co-designed classes, as long as you realise that the sub-class/super-class relationship is not the same as in arbitrary inheritance.
> In fact the design is more complicated, it implements the abstract > factory pattern providing two factory methods. There is also a subclass > of each of the four classes which provides additional functionality > again, each subclass implementing a new interface. See the above knee-jerk reaction. This complexity of inheritance, while not obviously /wrong/, does suggest that you may be over-using subclassing. It hints that you may be using sub-classing where containment would do the job better -- and indeed your proposed design is moving towards containment in a small way. Maybe (just maybe) the uneasiness you feel is because you haven't gone far enough in that direction.
It's probably worth thinking about what would happen to your design if you removed the shared superclass altogether, and moved the common stuff into one or more helper objects which are uses /by/ your concrete classes (a generalisation of what you are already considering).
-- chris
Lionel - 13 Nov 2006 21:50 GMT > It's probably worth thinking about what would happen to your design if you > removed the shared superclass altogether, and moved the common stuff into one > or more helper objects which are uses /by/ your concrete classes (a > generalisation of what you are already considering). I will continue to think about this.
Lets say I have the following classes:
SuperClass
SubClass1 implements InterfaceA1, InterfaceB1
SubClass2 implements InterfaceA1, InterfaceB2
SubClass3 implements InterfaceA2, InterfaceB1
SubClass4 implements InterfaceA2, InterfaceB2
And of course the corresponding interfaces.
There is only one class that ever even knows about any of the sub-classes, that's the class which acts as an interface between the database and the rest of the application.
As for the rest of the application it relies on a few regular methods in and a few abstract methods in SuperClass for determining all the information required for performing all operations. There are three factory methods which construct instances of another superclass which again provides a similar hierarchy as above.
The reason for using this hierarchy is so that I never have to worry about the exact implementation in the rest of the application, it gets figured out once when retrieving and saving info from/to the database and that's it.
There was someone else working on the code for a period when I was away and they hadn't designed using any polymorphism. The result was incredibly ugly, there were 3-4 massive switch statements that did pretty much the same thing - errrr, I still shudder at how long it took me to figure out what was going on!
I said in another branch of this thread that I would work on a simplified example. Soon I'm going to be able to provide access to the source which would free me up a bit.
Background info: It's an open source GPL application for antibiotic dose optimisation. We are developing it at the University of Queensland in Australia. I'm the developer, there are two pharmacists that I work with who provide all the domain knowledge.
Lionel.
Daniel Pitts - 14 Nov 2006 00:08 GMT > > It's probably worth thinking about what would happen to your design if you > > removed the shared superclass altogether, and moved the common stuff into one [quoted text clipped - 16 lines] > > And of course the corresponding interfaces. So, the implementation of Interface B1 in Subclass1 is similar/different from the implementation in SubClass3?
If it is similar, then maybe you should have instead A1impl, B1impl, A2impl B2impl. Then, SuperClass would have a constructor(A, B) Although, if there are combinations of options, maybe they don't realy share a common base class, and you shouldn't use interfaces this way.
Perhaps a small snippet of the interfaces would help us help you.
Lionel - 14 Nov 2006 04:26 GMT Quick summary of the below. There is too much to explain, I've only really touch the simplest part. I won't be offended if you look at it and choose not to respond because it is difficult to provide everything necessary.
>>> It's probably worth thinking about what would happen to your design if you >>> removed the shared superclass altogether, and moved the common stuff into one [quoted text clipped - 18 lines] > So, the implementation of Interface B1 in Subclass1 is > similar/different from the implementation in SubClass3? They are very similar. Which is why I'm asking the question :).
> If it is similar, then maybe you should have instead A1impl, B1impl, > A2impl B2impl. I'm with you on that.
> Then, SuperClass would have a constructor(A, B) But here I think it falls down. A1 is more like the opposite of A2 so this design would mean that SuperClass would need some concept of who and what A and B are and what they do. I need to think about this more because you may have raised a possibility that I need to consider.
> Although, if there are combinations of options, maybe they don't realy > share a common base class, and you shouldn't use interfaces this way. Perhaps I can dispose of the interfaces. Let's take SubClass1 and SubClass2 that both implement InterfaceA1. The main time that this is useful is when I'm saving the data from the subclasses or when I'm creating new instances of the subclass from data from the database:
public static void saveClass(SuperClass superClass) { //save the common data ...
//look at what else needs saving if(superClass instanceof InterfaceA1) { saveInterfaceA1((InterfaceA1)superClass); } if(superClass instanceof InterfaceA2) { saveInterfaceA2((InterfaceA2)superClass); } //and so on }
private static void saveInterfaceA1(InterfaceA1 a1) { //construct sql query and save data such as a1.getInterfaceInt() }
Similarly when we are reading data from the database if we have an instance of SubClass1 then we know there is a table with an entry containing the data that can be set using the setters in InterfaceA1.
It's convenient to know that an implementation of InterfaceA1 contains certain methods.
> Perhaps a small snippet of the interfaces would help us help you. You are right. I'm going to have to figure out how I can give you more data before we continue. The problem is larger than what we have discussed so far and I need to portray the information without posting a ridiculous amount otherwise I am wasting your time.
A brief summary is that each interface is very simple, four or so methods. Say you have a drug, the input method can be intravascular or extravascular. One interface is IVDrug and contains setters and getters for minInfusionTime and maxInfustionTime stating the min and max time to give the drug into the vein. EVDrug has a rate constant Ka which describes how quickly the drug goes from tablet form in the stomach into the bloody. Ka can be expressed as a formula. There is also a variability on Ka, so it is accurate +- some percentage amount.
e.g.
interface IVDrug { public int getMinInfusionTime(); public int getMaxInfusionTime(); }
interface EVDrug { public String getKaEquation(); public double getKaVariability(); }
I'll continue:
interface TwoCompDrug { public String getVpEquation(); public double getVpVariability(); public String getQEquation(); public String getQVariability(); }
Actually the 4th interface doesn't exist, it would however be OneCompDrug but there is nothing special about it.
A subclass is either (One Compartment or Two Compartment) AND (intravascular or extravascular).
Overall my feeling is that the code is fairly nice. It's easy to read and understand and recent extensions were very simple to design and implement. My view is that even if this isn't the best design it is still highly readable. There is some slight duplication of code.
It's just in my best efforts to continue improving my design skills that I seek to see if there is a better approach. I haven't had the luxury of being taught this stuff. University courses introduce you to software patterns, OO design etc. but when it comes to practice it is a different story. I just want to make sure I do the best job possible.
Thanks for your comments
Lionel.
Daniel Pitts - 14 Nov 2006 04:53 GMT > Quick summary of the below. There is too much to explain, I've only > really touch the simplest part. I won't be offended if you look at it [quoted text clipped - 103 lines] > > Lionel. So, in the implementations of the methods you listed above, do those calculate a value, or are they simply accessors to a field? If they both are only simple accessors, I would suggest changing the "is a" into a "has a", and simple have concrete objects. I think that our questions here may have been focused on the wrong part of your program... Do you have a "instanceof" checks in your code? If so, then it sounds like you might need to rethink your (lack of) use of polymorphism. Interfaces should usually describe a set of actions on (behavoir of) an object.
I'd be interested in seeing the big picture, and making suggestions from there. I would suggest reading "Refactoring: Improving the Design of Existing Code" (Fowler, Beck, Brant, Opdyke, Roberts) http://www.amazon.com/Refactoring-Improving-Design-Existing-Code/dp/0201485672
Look up "Refactoring: Replace Type Code with State/Strategy" or "Refactoring: Replace Type Code with Subclasses" It sounds to me like you have two different concepts that should be seperated from the heirarcy altogether.
Lionel - 14 Nov 2006 23:05 GMT > So, in the implementations of the methods you listed above, do those > calculate a value, or are they simply accessors to a field? Accessors in the classes we've been talking about.
> If they both are only simple accessors, I would suggest changing the > "is a" into a "has a", and simple have concrete objects. Ok, that makes sense and is simple. I'm just curious. This means that the base class must know about its subclasses and implementations. If for some reason in the future some pharmacist figured out that some new model was appropriate, they would have to change the base class to tell it about additional fields/accessors.
> Do you have a "instanceof" checks in your code? Once when saving the class to the database. Never again after that.
> If > so, then it sounds like you might need to rethink your (lack of) use of > polymorphism. The specifics of what we are talking about there is no polymorphism, but amongst other polymorphic methods there are a bunch of abstract methods in the base class that must be implemented in subclasses. As I said, I've used the Abstract Factory pattern, there are three factory methods, we don't need to go into that though, I'm confident with that component as I've used a known design pattern. There is only the one instance where I have to worry about what the subclass type is and that is in the class that interfaces to the database.
> I'd be interested in seeing the big picture, and making suggestions > from there. Is your email address is correct then I will notify you when I come up with a more complete picture.
> I would suggest reading "Refactoring: Improving the Design > of Existing Code" (Fowler, Beck, Brant, Opdyke, Roberts) > http://www.amazon.com/Refactoring-Improving-Design-Existing-Code/dp/0201485672 Great, I got myself a copy and will have a read.
Lionel.
Daniel Pitts - 14 Nov 2006 23:58 GMT > > So, in the implementations of the methods you listed above, do those > > calculate a value, or are they simply accessors to a field? [quoted text clipped - 40 lines] > > Lionel. If you send a message to my e-mail, while technically correct, will probably result in your message going straight to the junk mail folder. Try sending to Lionel@, instead of googlegroupie@. Hopefully I'll remember you :-) You can also leave a message anywhere on http://forums.virtualinfinity.net/
Chris Uppal - 15 Nov 2006 11:51 GMT > Perhaps I can dispose of the interfaces. Let's take SubClass1 and > SubClass2 that both implement InterfaceA1. The main time that this is [quoted text clipped - 14 lines] > //and so on > } It's starting to sound as if you are using the interfaces as something very like metadata -- it tells things (mostly the DB code) what "features" each drug has. If that's the case then maybe you would find it better to make that concept explicit, so that you can ask each drug for its list of features, and then ask the features to save themselves to the DB.
By "explicit" I mean that you represent the feature at the OO level, as real objects with real behaviour, rather than by hacking around with inheritance.
BTW, I find it rather odd that something outside the object is responsible for writing it to the DB -- surely the object itself is best placed to know what should be written ?
-- chris
Lionel - 15 Nov 2006 21:58 GMT [snip] comments here noted and considered.
> BTW, I find it rather odd that something outside the object is responsible for > writing it to the DB -- surely the object itself is best placed to know what > should be written ? If every object had to know about the database and how to construct SQL queries and save themselves the code would become a mess. The solution I use makes a very nice interface in which one class is responsible for the database whether that be MySQL, Access, PostGreSQL etc.
This is common practice AFAIK.
Lionel.
Tom Forsmo - 15 Nov 2006 23:46 GMT Hi
It sound to me that you are trying to model a class hierarchy that is designed around variant properties. I think it would be better to use composition instead. I don't quite understand all of your design yet, but as an example lets take the DB saveClass functionality.
By using composition and interfaces you could for example do the following. Lets say you have a class named Drug which is composed of a list of drug properties. The drug properties list would hold objects that implements a saveToDB() method. So, to save the list of properties to a db you would only need to iterate the list and call the saveToDB(). Here is a code example that only considers the db storage aspect.
interface DbStorage { public int saveToDb(Connection dbConn); }
public class DrugPropertyA implements DbStorage, PropertyA {
public int saveToDb(Connection dbConn) { // details on how to save a DrugProperty for the // property A class type. } }
public class DrugPropertyB implements DbStorage, PropertyB {
public int saveToDb(Connection dbConn) { // details on how to save a DrugProperty for the // property B class type }
}
public class Drugs {
List<DbStorage> prop = new ArrayList();
public List<DbStorage> getDrugProperties() { return prop; } }
public class DbManager {
public int storeDrugs(List<DbSTorage>) {
for(DbStorage e : prop) { e.saveToDb(dbcon); } } }
This way you don't have to consider the type of the properties. I am not saying this works for your design, but I think you should be looking in that direction for a solution.
You could extend this by doing
public class Drug {
protected String PropertyName; ... }
public class DrugA extends DrugProperty implements DbStorage, PropertyA { ... }
But I suspect that's what you already have done.
>>> Lets say I have the following classes: >>> [quoted text clipped - 9 lines] >>> >>> And of course the corresponding interfaces. I am assuming what you are talking about above is actually the specific example below, right?
> interface IVDrug { > public int getMinInfusionTime(); [quoted text clipped - 14 lines] > public String getQVariability(); > } I am not sure of exactly how to do this, because the problem is that you have different methods for different drugs. But my question is: could you express this in a different way? My thought is that composition is the way to go here as well, but in a different way. For example, at some point you need to know what specific methods you need to call to get what you want. Could the properties be expressed by a hash instead, which only contained what was relevant for the current drug? Its not very OO I suppose, but I think its a better way. Or if how you use these methods is done i such a way that you could use an exec()/calc() method in each drug/property instead, which returned the final result. Or providing the necessary arguments to the method?
E.g.
public interface PropertyCalculation { public float calc(int amount, int something); }
public class PropertyA implements PropertyCalculation {
public float calc(int amount, int something) { return (vpEquation * amount / something) / qeEquation / vpVariability); } }
public class Drug {
calculateX() { for(PropertyCalculation c : prop) { xValue += e.exec(); } } }
This was just an example, which might not even be relevant, but its difficult when not having all the details.
tom
Free MagazinesGet these publications absolutely FREE for up to 12 months. There are no hidden fees and no obligation. Simply choose a title, complete the application form and submit it. Read more ...
|
|
|