Refactoring da classe de conexão

5 respostas
franciscoh

Senhores,
Tenho uma classe de conexão puro em java, sem nenhum framework, apenas o Driver de conexão é usado.
Ela está funcionando perfeitamente, mas o intutito é fazer uma classe totalmente desacoplada e com alta coesão, sendo assim,
coloco a classe aqui para vocês darem uma olhada, e se possível opinarem,com críticas e sugestões, como ficaria melhor?
E ficará para futura referência para a comunidade desenvolvedora.
Sem mais delongas, o código:

package repository;

import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.util.ArrayList;
import java.util.List;

final public class SQLDao {
	
	// DATABASE CONFIGURATION
	private final static String DBURL = "jdbc:mysql://SERVERADREES/DATABASE";
	private final static String DRIVER = "com.mysql.jdbc.Driver";
	private final static String USER = "USER";
	private final static String PASSWD = "PASSWD";

	//Private intern attributes
	private static SQLDao instance;
	private Connection connection;
	private static PreparedStatement preparedStatement;
	private static int i = 0;
	
	/**
	 * Singleton pattern
	 */
	private SQLDao() {
		try {
			connection = getConnection();
		} catch (Exception exception) {
			exception.printStackTrace();
		}
	}

	/**
	 * 
	 * @return a instance of SQLDao, part of the Singleton pattern
	 */
	static SQLDao getInstance() {
		i = 0;
		if (instance == null) {
			instance = new SQLDao();
		}
		return instance;
	}

	/**
	 * 
	 * @return A connection open and ready;
	 * @throws SQLException Possible errors generated by a bad configuration of Database Server;
	 * @throws ClassNotFoundException Possible errors generated by a bad driver properly configuration;
	 */
	private static Connection getConnection() throws SQLException {
		try {
			Class.forName(DRIVER);
			return DriverManager.getConnection(DBURL, USER, PASSWD);
		} catch ( ClassNotFoundException exception ) {
			throw new SQLException("The driver is properly configured?\n " + exception.getMessage());
		} catch ( SQLException exception ) {
			throw exception;
		}
	}
	
	/**
	 * 
	 * @param parameter Parameter to be added to the query; 
	 * @throws SQLException Possible errors generated by a bad object;
	 */
	private static void addParameter( Object parameter ) throws SQLException {
		preparedStatement.setObject(++i, parameter);
	}

	/**
	 * 
	 * @param query The query to be executed in the database;
	 * @param keys Variables to be replaced in the query,
	 *        adds here the sanitization of queries;
	 * @return ResultSet containing the query result;
	 * @throws SQLException Possible errors generated by a bad query;
	 */
	ResultSet select( String query, Object ... keys ) throws SQLException {
		try {
		if ( connection.isClosed() )
			connection = getConnection();
		
			preparedStatement = connection.prepareStatement(query);
			for ( Object parameter : keys )
				addParameter(parameter);
			return preparedStatement.executeQuery();
		} catch ( SQLException exception ) {
			throw exception;
		} finally {
			i = 0;
			preparedStatement.clearParameters();
		}
	}
	
	/**
	 * 
	 * @param query The query to be executed in the database;
	 * @return ResultSet containing the query result;
	 * @throws SQLException Possible errors generated by a bad query;
	 */
	ResultSet select( String query ) throws SQLException {
		try {
		if ( connection.isClosed() )
			connection = getConnection();
		
			preparedStatement = connection.prepareStatement(query);
			return preparedStatement.executeQuery();
		} catch ( SQLException exception ) {
			throw exception;
		} finally {
			i = 0;
			preparedStatement.clearParameters();
		}
	}

	/**
	 * 
	 * @param query The query to be executed in the database;
	 * @param keys Variables to be replaced in the query,
	 *        adds here the sanitization of queries; 
	 * @return integer value that represents the number of rows affected
	 * @throws SQLException Possible errors generated by a bad query;
	 */
	int execute( String query, Object ... keys ) throws SQLException {
		try {
		if (connection.isClosed())
			connection = getConnection();
		
			preparedStatement = connection.prepareStatement(query);
			for ( Object parameter : keys )
				addParameter(parameter);
			return preparedStatement.executeUpdate();
		} catch ( SQLException exception ) {
			throw exception;
		} finally {
			i = 0;
			preparedStatement.clearParameters();
		}
	}
	
	/**
	 * 
	 * @param query The query to be executed in the database;
	 * @return integer value that represents the number of rows affected
	 * @throws SQLException Possible errors generated by a bad query;
	 */
	int execute( String query) throws SQLException {
		try {
		if (connection.isClosed())
			connection = getConnection();
		
			preparedStatement = connection.prepareStatement(query);
			return preparedStatement.executeUpdate();
		} catch ( SQLException exception ) {
			throw exception;
		} finally {
			i = 0;
			preparedStatement.clearParameters();
		}
	}

	/**
	 * 
	 * @param query The query to be executed in the database;
	 * @param keys Variables to be replaced in the query,
	 *        adds here the sanitization of queries; 
	 * @return A list of integer values that represents the IDs affected 
	 * @throws SQLException Possible errors generated by a bad query;
	 */
	List<Integer> executeRetriveIdAffected( String query, Object ... keys ) throws SQLException {
		try {
		if ( connection.isClosed() )
			connection = getConnection();

			preparedStatement = connection.prepareStatement( query, java.sql.Statement.RETURN_GENERATED_KEYS );
			for ( Object parameter : keys )
				addParameter(parameter);
			ResultSet retrivedKeys = preparedStatement.getGeneratedKeys();
			List<Integer> dataBD = new ArrayList<Integer>();
			while ( retrivedKeys.next() )
				dataBD.add(retrivedKeys.getInt(1));
			return dataBD;
		} catch ( SQLException exception ) {
			throw exception;
		} finally {
			i = 0;
			preparedStatement.clearParameters();
		}
	}
	
	/**
	 * 
	 * @param query The query to be executed in the database;
	 * @return A list of integer values that represents the IDs affected 
	 * @throws SQLException Possible errors generated by a bad query;
	 */
	List<Integer> executeRetriveIdAffected( String query ) throws SQLException {
		try {
		if ( connection.isClosed() )
			connection = getConnection();
		
			preparedStatement = connection.prepareStatement( query, java.sql.Statement.RETURN_GENERATED_KEYS );
			ResultSet retrivedKeys = preparedStatement.getGeneratedKeys();
			List<Integer> dataBD = new ArrayList<Integer>();
			while ( retrivedKeys.next() )
				dataBD.add(retrivedKeys.getInt(1));
			return dataBD;
		} catch ( SQLException exception ) {
			throw exception;
		} finally {
			i = 0;
			preparedStatement.clearParameters();
		}
	}
	
	
	/**
	 * 
	 * @throws SQLException Possible errors generated by a already closed connection;
	 */
	void dispose() throws SQLException {
		if (preparedStatement != null)
			preparedStatement.close();

		if (!connection.isClosed())
			connection.close();
	}

}

O que não gosto nesse código?
Está violando o princípio DRY(Dont’t repeat Yourself)

5 Respostas

Ataxexe

Bom, de início eu arrancaria o singleton e colocaria os métodos públicos.

Outra coisa é fazer a inicialização da classe do driver apenas uma vez, não há a necessidade de se fazer mais de uma vez. Isso é devido a um padrão de que as classes de driver JDBC se registrem no DriverManager com um bloco estático, por isso que fazemos o Class.forName nela e, como mágica, conseguimos usar o driver. Nas versões mais novas do JDBC não é necessário fazer isso (basta checar na pasta META-INF/services do jar por um arquivo de nome java.sql.Driver).

Outro ponto é que sua classe não me parece thread safe pois o contador de parâmetros é parte da classe. Com isso, se duas threads fizerem uma conexão ao banco, você pode ter umas exceções difíceis de depurar.

[EDIT]
Uma outra questão. Sua classe está com duas responsabilidades: buscar conexões e executar sql. Acho que seria mais bacana você ter uma classe para buscar as conexões e outra para executar os sqls. Assim você desacopla mais as coisas.

franciscoh

Primeiro, obrigado por responder Ataxexe,
verdade, acabei de verificar, não precisamos registrar o DRIVER a todo momento, legal não sabia disso.
Quanto ao desacoplamento você tem, razão, deveríamos ter uma classe separada para prover a Conexão com o Banco de Dados.
Já sobre o multi-thread, pensando em um ambiente que dê suporte à concorrência, precisáriamos de muito mais que essa classe, já que se, uma aplicação é multi-thread, teríamos que pensar em um banco de dados que dê suporte à concorrência(talvez com timestamps indicando lock da table), e modificaria a instrução de:

SQLDao.execute("INSERT INTO TABLE VALUES(?,?)",var1,var2);

para:

SQLDao.execute("INSERT INTO TABLE VALUES(?,?)");
SQLDao.addParameter(++i,var1);
SQLDao.addParameter(++i,var2);

Sendo assim, prejudicaria a legibilidade do código, pois se tivesse um insert com 10 parâmetros, teria que adicionar também 10 parâmetros.
Talvez poderíamos colocar o bloco de código dentro de um instrução syncronized.

Ataxexe

Acredito que 99% dos bancos de dados utilizados em produção suportem conexões simultâneas, então é muito melhor que você tenha uma interface expondo os seus métodos e uma classe que decore uma instância com métodos sincronizados (semelhante ao que o Collections#synchronizedList faz).

Depois de fazer isso, você irá perceber que pode fazer uso de um Datasource (o mais recomendado se for usar um servidor de aplicações) e verá claramente a necessidade de separar a conexão do executor de sql. Então você começará a componentizar as coisas criando um componente para retornar a conexão.

Depois você irá ver a necessidade de criar algo do tipo PreparedStatementPopulator, que pegaria um objeto e popularia um PreparedStatement. Mais uma coisa desacoplada da classe :slight_smile:

Depois você irá querer fazer o mesmo na volta: um componente que popula um objeto usando um ResultSet como base.

Você vai perceber, conforme for refatorando, que acabará criando algo já existente no mercado. É nessa hora que você verá que é muito melhor usar um framework já pronto pra isso do que tentar fazer algo parecido.

Note que não estou te desencorajando de forma alguma a prosseguir com sua classe de conexão. Eu aprendi muito (muito mesmo) reinventando a roda por esporte com a finalidade de entender como as coisas funcionam, mas não uso nada disso em projetos, apenas uso para aprimorar meus conhecimentos.

N

Talvez eu tenha fugido um pouco do assunto mas você pode como já foi dito separar as tarefas de realizar instruções sql das tarefas de obter coneção e implementar um pool de conexões. Tive a oportunidade de usar o componente dbcp do projeto apache e recomendo.

http://commons.apache.org/dbcp/

O legal do dbcp é que me causou curiosidade em estudar a implementação de pools também.

Abs

franciscoh

Obrigado pela informação nfrpaiva, darei uma olhada no componente dbcp.
Ataxexe, concordo com o que disse.É importante para aprendermos, ainda mais para mim, que estou inciando em java.
Mas para fins de produção, um framework talvez seria mais plausível.
Coloco aqui, mais um refactoring da classe (não sei se ficou melhor ou pior, depende da opinião de cada um), de acordo com alguns itens levantados.
Changelog:
* Foi implementado o padrão singleton apenas na classe de conexão.Desta forma eu teria apenas uma conexão ao banco de cada vez;
* A responsabilidade de conexão, foi desacoplada da responsabilidade de execução de querys;
* Foram eliminados os códigos repetidos, DRY.

Alguns itens deixaram a desejar, mas vou parar por aqui, pois como o Ataxexe disse, o refactoring nunca terminaria, e chagaríamos a algum desarcordo(de uma versão ser melhor ou pior que a outra) ou então, chegaríamos a um princípio de algum framework existente no mercado.
Itens que deixaram a desejar:
* Realização de boxing e unboxing, prejudicando a performance;
* Não dâ suporte a aplicações multi-threads.

Classe Conexão:
package repository;

import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.SQLException;

public class SqlConnection {

	private final static String DBURL = "jdbc:mysql://SQLADDRESS/DATABASE";
	private final static String DRIVER = "com.mysql.jdbc.Driver";
	private final static String USER = "USER";
	private final static String PASSWD = "PASSWD";
	private static SqlConnection instance;
	
	private SqlConnection()
	{ }
	
	static SqlConnection getInstance() {
		synchronized (SqlConnection.class) {
			if (instance == null) {
				instance = new SqlConnection();
			}	
		}
		return instance;
	}
	
	Connection getConnection() throws SQLException {
		try {
			return DriverManager.getConnection(DBURL, USER, PASSWD);
		} catch ( SQLException exception ) {
			throw exception;
		}
	}
	
	SqlConnection registerDriver() {
		try {
			Class.forName(DRIVER);
		} catch ( ClassNotFoundException exception ) {
			exception.printStackTrace();
		}
		return getInstance();
	}
	
}
Classe responsável pela execução de querys.
package repository;

import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
import java.util.ArrayList;
import java.util.List;

final public class SQLDao {
	
	SqlConnection database;
	private Connection connection;
	private static PreparedStatement preparedStatement;
	private static int i;
	boolean returnGeneratedKey;
	private List<Integer> generatedKeys;
	
	public SQLDao() {
		try {
			database = SqlConnection.getInstance();
			connection = database.getConnection();
			returnGeneratedKey = false;
			generatedKeys = null;
			i = 0;
		} catch ( SQLException exception ) {
			exception.printStackTrace();
		}
	}
		
	public List<Integer> getGeneratedKeys() {
		try {
			return generatedKeys;
		} finally {
			this.generatedKeys = null;
		}
	}

	private static void addParameter( Object parameter ) throws SQLException {
		preparedStatement.setObject(++i, parameter);
	}

	ResultSet select( Object ... keys ) throws SQLException {
		if (!( keys[0] instanceof String ))
			throw new SQLException("The first argument need to be a String query!");
		try {
			preparedStatement = connection.prepareStatement((String)keys[0]);
			for ( int i = 1; i < keys.length; i++ )
				addParameter(keys[i]);
			return preparedStatement.executeQuery();
		} catch ( SQLException exception ) {
			throw exception;
		} finally {
			i = 0;
			preparedStatement.clearParameters();
		}
	}

	int execute( Object ... keys ) throws SQLException {
		if (!( keys[0] instanceof String ))
			throw new SQLException("The first argument need to be a String query!");
		try {
			int retorno;
			if(!returnGeneratedKey) {
				preparedStatement = connection.prepareStatement((String)keys[0]);
				for ( int i = 1; i < keys.length; i++ )
					addParameter(keys[i]);
				retorno = preparedStatement.executeUpdate();
			} else {
				generatedKeys = new ArrayList<Integer>();
				preparedStatement = connection.prepareStatement((String)keys[0],Statement.RETURN_GENERATED_KEYS);
				for ( int i = 1; i < keys.length; i++ )
					addParameter(keys[i]);
				retorno = preparedStatement.executeUpdate();
				ResultSet retrivedKeys = preparedStatement.getGeneratedKeys();
				while ( retrivedKeys.next() )
					generatedKeys.add(retrivedKeys.getInt(1));
			}
			return retorno;
		} catch ( SQLException exception ) {
			throw exception;
		} finally {
			i = 0;
			returnGeneratedKey = false;
			preparedStatement.clearParameters();
		}
	}

	void saveGeneratedKey( ) {
		returnGeneratedKey = true;
	}
	
	void disposePreparedConnection() throws SQLException {
		if (preparedStatement != null)
			preparedStatement.close();
	}
	
	void disposeConnection() throws SQLException {
		if (connection != null)
			connection.close();
	}

}
Criado 2 de julho de 2012
Ultima resposta 3 de jul. de 2012
Respostas 5
Participantes 3