The Nightstar Zoo

Nightstar IRC Network - irc.nightstar.net
It is currently Sun Apr 30, 2017 11:40 am

All times are UTC - 6 hours [ DST ]




Post new topic Reply to topic  [ 5 posts ] 
Author Message
 Post subject: refactor(this);
PostPosted: Sat Apr 29, 2006 5:42 pm 
Offline
Nightstar Graveyard Daemon
User avatar

Joined: Mon Jun 03, 2002 8:30 pm
Posts: 1071
Location: Wouldn't you rather observe my Velocity?
*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*.

Code:
   protected ActivityNameDb loadActivityName(int id) {
      ActivityNameDb thisActivityName = null;
      String queryString;
      DbQuery query;
      DbResultSet rs;
      
      queryString = "SELECT * FROM activity_name WHERE id=" + id;
      query = new DbQuery(queryString);
      
      rs = DbAccess.query(query);
      try {
         if (rs.next() ) {
            DbRow row = new DbRow(rs);
            thisActivityName = new ActivityNameDb(row);
         }
      } catch (SQLException se) {
         se.printStackTrace();
         fail( "SQLException trying to load activity_name id=" + id);
      }
      return thisActivityName;
   }


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
Code:
FooDb = loadDbObject( "Foo", 42 );


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?


Top
 Profile  
 
 Post subject:
PostPosted: Sat Apr 29, 2006 8:39 pm 
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:
Code:
class ActivityNameDb extends <whatever> implements Identifiable, <etc.> {
   public static final String TABLE_NAME = "activity_name";
   .
   .
   .
   .
}


Your factory method takes a Class object, extracts the TABLE_NAME field to construct the SQL, then returns an Identifiable Object:
Code:
   protected Identifiable loadDbObject(Class class, int id) {
      Identifiable thisIdentifiable = null;
      String queryString;
      DbQuery query;
      DbResultSet rs;
     
      if (! class.isAssignableFrom(Identifiable)) {
         /* class is not an instance of Identifiable. */
      }

      queryString = "SELECT * FROM " + class.getDeclaredField("TABLE_NAME").toString() + " WHERE id=" + id;
      query = new DbQuery(queryString);
     
      rs = DbAccess.query(query);
      try {
         if (rs.next() ) {
            DbRow row = new DbRow(rs);
            thisIdentifiable = (Identifiable) class.newInstance(row);   /* FIXME */
         }
      } catch (SQLException se) {
         se.printStackTrace();
         fail( "SQLException trying to load " + class.getDeclaredField("TABLE_NAME").toString() + " id=" + id);
      }
      return thisIdentifiable;
   }


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.


Top
  
 
 Post subject:
PostPosted: Mon May 08, 2006 10:16 am 
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.


Top
  
 
 Post subject:
PostPosted: Sat May 27, 2006 3:09 am 
Offline
Knight of Daisies, Tulip Slayer
User avatar

Joined: Sat May 11, 2002 3:03 am
Posts: 1621
Location: Sector ZZ9 Plural Z Alpha
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.)


Top
 Profile  
 
 Post subject:
PostPosted: Sat May 27, 2006 4:09 am 
Offline
Energizer Bunny
User avatar

Joined: Wed May 22, 2002 12:24 am
Posts: 1634
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


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 5 posts ] 

All times are UTC - 6 hours [ DST ]


Who is online

Users browsing this forum: No registered users and 1 guest


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum

Search for:
Jump to:  
Powered by phpBB® Forum Software © phpBB Group