The Nightstar Zoo

*sigh* Here's a bit of code that is making me crazy. Ideas on how to refactor are welcome. The problem is that this is in Java, and the solution must also be in Java. *SIGH*.



Now, this code is more or less fine as is, though I'll entertain cleanups in here as well. Here's the problem:

• This code is in a DbManager class, which is a factory for about 70 different types of classes.
• There are about 20 case-and-pasted copies of this method that differ only by the class of object created/returned and the table name it is loaded from. (There would be 1 per table if we were exhaustively complete, and each week we seem to add a new one when we need it. Hence my desired to clean this up before it gets out of hand (even more).)

Now, here are the current interface assumptions... Feel free to bash new interfaces in if necessary, because I think this architecture is crappy as hell but I've been too tired/lazy/busy to fix it yet.

• ActivityNameDb and all it's peers extend interface Identifiable, which has "int getId()" and "void setId(int id)".
• All these classes implement a constructor that takes a DbRow object. They fish out the params they are interested in.
• None of these classes talk to the database directly. For all they know, DbRow is just a hash of initializer values.

I have no clue about how to solve this in Java. In PHP, I built a framework that new about classnames and the tablenames they came from so you could say


In C you could write a macro to generate the function type you needed. In C++ you could write a template, though you might need to pass in an instance of the class to make it differentiate correctly. In Ruby, Python or perl, you don't have static typing so the problem simply doesn't exist.

In Java... the best solution I've come up with is just pasting the code out 50 times. Blech.

Ideas?

In Java, classes can also be represented as objects (subclasses of java.lang.Class). So, (in theory anyway) you could do something like this:

First, each class should define a public String constant that identifies the table name it gets its data from:


Your factory method takes a Class object, extracts the TABLE_NAME field to construct the SQL, then returns an Identifiable Object:


Notice the "FIXME" in the above code. class.newInstance() doesn't actually take any arguments. In real code, you will have to look up the proper constructor with class.getConstructor(), then call the Constructor object's newInstance() method.

Not having a Java compiler installed, I don't know if the above code will even compile. In particular, "class.isAssignableFrom(Identifiable)" should probably be changed to "class.isAssignableFrom(Identifiable.getClass())". But I'm not sure, so you're on your own to find the proper syntax.

Hope this helps.

Edit: Now that I think about it, it would probably be better (from an abstraction standpoint) to add a "static String getTableName()" method to Identifiable that your subclasses could implement, rather than using a public constant. That way, if you create a new class and forget to add the table name, it's a compile error rather than a run-time error.

Sorry for the delay on this, but I don't think I've checked this forum in a while. Anyway, assuming I'm not still asleep this morning, I can see three really simple solutions.

First off, you can simply use Object as the return type. Since Java uses a singly-rooted object hierarchy, this would cover all non-primitive cases (unlike in C++, where you often have objects with no common superclasses). However, this won't work with primitive types (though you can of course use the equivalent wrapper classes), you'll need to downcast the objects, and you may need to use instanceof() to check the correct type first. In the function itself, you can use a HashMap or some other form of lookup collection to select the correct table name.

Second, you can make this method and the class it is in abstract, and create a subclass for each of the cases which handles the specific instances, calling the parent's method to handle the actual performance. I doubt that this would be much more acceptable than the current situation, however, as there would still be a lot of redundancy, but it would at least reduce the amount of duplication.

Finally, if you are using the newest version of Java (1.5), you can use generics, which work very similarly to C++ templates. There are some differences, however, and you'll want to check to make sure that what you have in mind would work.

Not sure if I entirely grok what's going on here... It seems to me that your factory is there to instantiate objects in various families...

In which case, the Abstract Factory pattern might be useful. http://en.wikipedia.org/wiki/Abstract_factory_pattern

If this is not what you're doing, then feel free to ignore me.

If this is what you're doing, and Abstract Factory does indeed prove useful, then I have only one thing to say: IN YOUR FACE, VORN!

_________________
Fandemonium 2010 -- No Boundaries.
http://www.fandemonium.org
Friday - Sunday, August 6th - 8th, 2010
Nampa Civic Center - Nampa, Idaho (Only 20 minutes from the airport!)
(Idaho: It ain't just potatoes anymore.)

Nope.

The abstract factory method does not reduce duplication within a single class, which is the problem.

If you want to reduce this kind of duplication, you make a table, keyed on an enum, stuffed with the class to create, the query string, etc. Then you have to write an interface overlaying the creatable classes, and call a method with the enum as an argument.

Vorn