DAO methods and synchronized

Question

The following are methods I currently have in an Abstract DAO class. If there are concurrent calls, are they safe as they are or should synchronization be used? I know synchronization should be used if there is a reference to an attribute outside the scope of a method but it's unclear to me how should things be handled with an outside ressource.

public Connection getConnection() {
    // Call to singleton handling JDBC stuff
    return Database.getInstance().getCon();
}

public boolean isConnectionAvailable(){     
    if( getConnection() != null ){
        return true;
    }

    return false;
}

public PreparedStatement getPreparedStatement( String sqlStatement ){
    Connection connection = getConnection();
    PreparedStatement pS = null;

    if( connection != null ){
        try {
            pS = connection.prepareStatement( sqlStatement );
        } catch (SQLException e) {
            return null;
        }
    }

    return pS;
}

Edit: I might reformulate this question to include information on writing DAOs as it's what's important here.

Answer 1

If getCon() returns a new Connection each time it is called, or returns a ThreadLocal connection, then you are safe and there is no need to use synchronized

If you return the same connection to everyone you might still save in terms of synchronization, because there is no state in the connection that gets changed (in your current code). But you should avoid this practice. Consider a connection pool instead.

And a few notes on general design principles. DAOs form a separate layer. Each layer exists for a reason, not juts for the sake of having cool names. The DAO layer exists in order to abstract, or in other words - hide the database access from the services that use the DAO objects. In order to imagine it more clearly - the DAO has to be written in a way that if tomorrow you decide to switch from RDBMD storage (via JDBC) to XML storage, you should be able to do that by changing only the DAO objects and nothing else.

Answer 2

The JDBC Connection class is not guaranteed to be thread safe. If your Database.getInstance().getCon() method is always returning the same connection, then you will run into problems. If, however, it is using a pool such that each call to getInstance().getCon() returns a different connection you will be fine.

That said, if you are returning a different connection with each call to getCon() then the getPreparedStatement() won't work if you want two Prepared Statement calls to use the same connection (and same transaction).

I like Spring's JDBCTemplate class as a basis for my DAO classes.

Answer 3

Looks fine to me. The functions are Thread safe.

Answer 4

I don't agree with this implementation at all.

First, DAOs should be given their connection information by the services that own units of work and transactions.

Second, I don't see an interface.

Third, I don't see model or domain objects.

Fourth, prepared statements should only be part of the internal implementation. If they're leaking out of your DAO, you're doing it wrong.

Fifth, passing a prepared statement out of the object makes responsibility for closing it and cleaning up far less clear. Your DAO will die a resource leak death in no time.

Here's an interface for a generic DAO. You'll notice that it's all CRUD operations, with no mention of connnections or any interfaces from java.sql package:

package persistence;

import java.io.Serializable;
import java.util.List;

public interface GenericDao<T, K extends Serializable>
{
    T find(K id);
    List<T> find();
    List<T> find(T example);
    List<T> find(String queryName, String [] paramNames, Object [] bindValues);

    K save(T instance);
    void update(T instance);
    void delete(T instance);
}

You can go a long way with this. It's a better abstraction. The T is your business object type, and K is the primary key.

Answer 5

You shouldn't use the same connection in every thread. JDBC drivers aren't supposed to be thread safe. If you want a thread safe code, you should create one connection for every thread.

The rest of your code seems safe.

Answer 6

Look at how Spring does it, they have already figured out all this stuff and there is no need to re-invent it. Check out the petclinic sample code that is bundled with the full distribution of Spring, or (for a non-Spring approach) read the DAO chapter of the Bauer/King Hibernate book.

Definitely the DAO shouldn't be in charge of getting the database connection, because you will want to group multiple DAO calls in the same transaction. The way Spring does it is there's a service layer that gets its transactional methods wrapped in an interceptor that pulls the connection from the datasource and puts it in a threadlocal variable where the DAOs can find it.

Eating your SQLException and returning null is bad. As duffymo points out, letting the PreparedStatement get passed around with no assurance that it will ever get closed is very bad. Also, nobody should use raw JDBC anymore, Ibatis or spring-jdbc are better alternatives.