views:

847

answers:

8

The application I'm working on has a single class that maintains a database connection. All members of this class are static to enforce a singleton-like pattern, so the actual connection logic is performed in a static initializer block:

public class HibernateUtil {

    private static final SessionFactory sessionFactory;

    static {
        sessionFactory = new Configuration().configure().buildSessionFactory();
    }

    static void openSession() {
        //open session with sessionFactory
    }

    public static Session currentSession() {
        //return the currently open session
    }

    static void closeSession() {
        //close the currently open session
    }

}

However, the application now needs to open a database connection to a second database. The way this class in structured now, the only way to maintain the second connection while keeping the above pattern would be to create a second class (something like SecondHibernateUtil) and change the one line of configuration in the initializer block. That feels like a truly wasteful amount of copy/paste.

Can anyone suggest a way that I could rework this setup to maintain multiple connections simultaneously while not being overly destructive to the code already calling the existing methods?

+2  A: 

Maybe it shouldn't have been static in the first place.

Mehrdad Afshari
Agreed. Make the class non-static and add a static factory method to return a new instance.
Andrew Rollings
I was thinking of a lengthy answer, but this pretty much sums it up.
eljenso
This is not an answer, but a comment.
OscarRyz
Accurate thought. Sad but true, it should not have been static.
OscarRyz
I disagree. When there is not a good reason for making a static class, why you should do so?
Mehrdad Afshari
@Mehrdad. Your reasoning is correct, and I agree with you, but the answer you gave is not an answer, is a comment. And answer would've been: Substitute the static class with a non static implementation. etc. etc
OscarRyz
@Oscar, good point.
jimmyorr
As long as it doesn't make a difference, I prefer the shortest answer.
Mehrdad Afshari
Mutable statics (even if you call them singletons) are almost always a really bad idea. Parameterise from Above!
Tom Hawtin - tackline
A: 

There isn't really a good way, some times there is no real choice other than just copy the class whole sale and live with the duplication.

I know this sounds really bad but hell how many times has this class actually changed since it was copied from the hibernate documentation?

I'm sure somebody is going to suguest converting the whole app to have spring managed SessionFactory and all that stuff, but unless your app is Hibernate "Hello World" then I would guess that this might take a bit longer than a couple of hours.

Gareth Davis
"no real choice" - I think there is. This class shouldn't be static. "copy the class whole sale" is not DRY.
duffymo
as I said *sometimes* there isn't a choice ... if it is the case that making it none static means refactoring half of your app then in the real world there really isn't much choice.
Gareth Davis
+2  A: 

Keep the static methods and let them delegate to corresponding instance methods of a default instance.

For the secondary databases allow to create more instances by whatever means and access them via their instance methods.

Then you can chip away at the callers of the static methods, and when they are all gone remove the static methods.

starblue
A: 

Learn Spring. If you're already using Hibernate, it won't be a stretch to learn enough Spring to handle this problem. The Spring Source folks have already solved this problem better than you ever will.

Spring transactions are valid for JDBC, Hibernate, iBatis, or any other persistence technology that Spring supports. If you're already using Spring, don't reinvent what they've already done better. Use their transactions.

If you have more than one database, and you perform writes to both as a single unit of work, you know enough to use the XA drivers for both, right?

duffymo
A: 

There are two possible answers; let's look at both.

One is to convert it to a non-static. Arguably this would have been better than a static interface in the first place, since you have a concrete object (SessionFactory) behind the interface. The advantage of doing this is that you defer the creation of the SessionFactory till you need it (and never create it if you don't). Make a static method to get a default HibernateUtil object, creating it if one doesn't exist. You may or may not want to permit a second HibernateUtil.

The other way is to create an entirely new static object HibernateUtil2 which delegates to HibernateUtil where necessary. However note that these won't be related to each other in any inheritance-like manner, because statics can't be used in interfaces.

I would recommend the first route, all else being equal, but your specific case may warrant the second.

DJClayworth
+1  A: 

Well, this is the right time to make things properly.

There is no way to add a second connection AND keep the pattern you've described.

Instead you may try to get things right.

You have two options.

First option. Keep the mess and make it a little worst ( without telling your clients )

  1. Create an utility class that handle the connections for you. This time don't limit to only two. Use a map instead and don't make it static.
  2. Change your static class use that new utility object.
  3. Create your new connection utility ( HibernateUtility2) and use that new utility object too.

    // Create a new utility class using a map instead.
    class HibernateUtility { 
    
    
    
    private Map<String, SessionFactory> factories;
    private HibernateUtility instance;
    
    
    public  static HibernateUtility getInstance() { 
        if( instance == null ) { 
        instance = new HibernateUtility();
        }
        return instance;
    }
    private HibernateUtility() {}
    
    
    private void check( String whichConnection ) { 
        if( !factories.containsKey( whichConnection ) ) {
            // start the connection
            factories.put( whichConnection, new Configu..
            //... etc etc ().configure().buildSessionFactory() );
        }
    }
    
    
    void openSession( String whichConnection ) {
        check( whichConnection );
        //open session with sessionFactory
        factories.get( whichConnection ).open(); //etcetc
    }
    
    
    Session currentSession( whichConnection ) {
        check( whichConnection );
        //return the currently open session
        factories.get( whichConnection ) .....
    }
    
    
    void closeSession( String whichConnection ) {
        check( whichConnection );
        //close the currently open session
        factories.get( whichConnection );
    }
    
    } //------------------------------------------ // Make your existing class call that class public class HibernateUtil {
    static void openSession() {
        //open session with sessionFactory
        HibernateUtility.getInstance( "one" ).openSession();
    }
    
    
    public static Session currentSession() {
        //return the currently open session
        HibernateUtility.getInstance( "one" ).currentSession();
    }
    
    
    static void closeSession() {
        //close the currently open session
        HibernateUtility.getInstance( "one" ).closeSession();
    }
    
    } //------------------------------------------ // Make the new connection use that class too. public class HibernateUtil {
    static void openSession() {
        //open session with sessionFactory
        HibernateUtility.getInstance( "two" ).openSession();
    }
    
    
    public static Session currentSession() {
        //return the currently open session
        HibernateUtility.getInstance( "two" ).currentSession();
    }
    
    
    static void closeSession() {
        //close the currently open session
        HibernateUtility.getInstance( "two" ).closeSession();
    }
    
    }

This way the existing client code won't break.

Second option. Clean up the mess and modify the code accordaingly.

In this second option you create an object very similar to the HibernateUtility described before, but this time you don't add the methods openSession nor closeSession, which don't belong to that interface.

Using this non static method let's you create different instances that could connect to different configurations.

You just just use the currentSession that if the session doesn't exists creates a new one, and receive as parameter an identifier ( much like "one" and "two" in the previous sample )

In this second option you have to change your client code to use the rigth implementation.

Note: Somethings wrong with the code formatting in SO, it took me some time to format it the way it is, and it doesn't look any good :(

OscarRyz
@duffymo: I wanted to express the client code wont stop , won't breake. :) :) :) naahhh just kidding :"P thanks for the edit!...
OscarRyz
+1  A: 

I looked at this question a long time ago and because of the exact question you are posing, I decided never to make static classes like this. There are actually 2 problems with it...

First it's not reusable (as you noticed) and fixing it is a tough refactor.

Second, you can't have a second instance (Seems like programmers assume there is only one of a thing almost as much as they assume they will never need to change the first two digits of a date!)

If you decide you want a second instance with a factory pattern (or something like spring), it's a trivial change--just pass in a variable to the factory to indicate which instance you want--but to refactor from a static class is just a bitch.

You should even be careful not to abuse factory patterns. If you really lean in that direction, learn and embrace spring--it'll combine with your natural tendencies to improve your code by bounds.

Bill K
A: 

The least disruptive way to fix this would be to add 2nd static variable to the class, initialize 2nd SessionFactory in the static block and then modify "currentSession" static method to have a String parameter to select proper instance based on you business/code logic.

Rocket Surgeon