Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I got three classes for MySQL Database access/manipulation,

  • First : Conector. It has got methods for connecting, disconnecting, querying and updating db.
  • Second : ConectorCliente. It simply extends first and got its constructor receiving user, password, host, port and db.
  • And third one I have written has several different methods per each DB, depending on data which will be needed to be selected, updated or deleted.

These are my classes in the mentioned order:

First class

public class Conector {

    private Connection  connection  = null;
    private Statement   statement   = null;
    private ResultSet   set         = null;

    String host;
    String port;
    String login;
    String password;
    String url;

    public Conector (String login, String password, String db, String host, String port) {
        this.login = login;
        this.password = password;
        this.host = host;
        this.port = port;
        url = "jdbc:mysql://"+host+":"+port+"/"+db;
        Conectar ();
    }

    public void Desconectar () {
        connection = null;
    }

    private void Conectar() {
        try {
            DriverManager.registerDriver(new com.mysql.jdbc.Driver ());
        } catch (SQLException ex) {
            Logger.getLogger(Conector.class.getName()).log(Level.SEVERE, null, ex);
        }
        try {
            connection = DriverManager.getConnection(url, login, password);
            statement = connection.createStatement();
        }

        catch (SQLException e) {
            Logger logger = Logger.getLogger(Conector.class.getName());
            logger.log(Level.SEVERE, e.getSQLState(), e);
        }
    }

    public ResultSet Query(String query){

        try {
            statement = connection.createStatement();
            set = statement.executeQuery(query);
        }
        catch (Exception e) {
            System.out.println("Exception in query method:\n" + e.getMessage());
        }
        return set;
    }

    public boolean Update (String update) {

        try {
            statement = connection.createStatement();
            statement.executeUpdate(update);

        }
        catch (SQLException e) {
            System.out.println("Exception in update method:\n" + e.getMessage());
            return false;
        }

        return true;
    }

    public void cierraConexion(){

        try {
            connection.close();
            connection = null;
        }
        catch(Exception e)         {
            System.out.println("Problema para cerrar la conexión a la base de datos ");
        }
    }

}

Second class

public class ConectorCliente extends Conector {

    public ConectorCliente(String login, String password, String db, String host, String port)  {

        // Con super llamamos y ocupamos el constructor de la clase padre
        super(login, password, db, host, port);

    }
}

Example of third class

public class InteraccionDB {


    private static final ConectorCliente conector = new ConectorCliente("user", "pass","db", "127.0.0.1", "3306");


    public static int obtenerNumRegistros() {

        ResultSet resultado = conector.Query("SELECT COUNT(*) FROM tweets;");

        try {
            while (resultado.next()) {

                return resultado.getInt("COUNT(*)");


            }
        } catch (SQLException ex) {
            Logger.getLogger(InteraccionDB.class.getName()).log(Level.SEVERE, null, ex);
        }

        return -1;
    }

    public static void insertarTweet(String id) {


        conector.Update("INSERT INTO tweets (id) VALUES ('"+id+"')");
    }


    public static void eliminarTweet(String id) {

        conector.Update("DELETE FROM tweets WHERE id = '"+id+"'");        
    }

    public static HashMap<String, Integer> obtenerHoraTweet(String id) {

        HashMap<String, Integer> resultados = new HashMap<>();

        ResultSet resultado = conector.Query("SELECT hora, min FROM tweets WHERE id = '"+id+"'");

        try {
            while (resultado.next()) {

                resultados.put("Hora", resultado.getInt(1));
                resultados.put("Minuto", resultado.getInt("min"));

            }
        } catch (SQLException ex) {
            Logger.getLogger(InteraccionDB.class.getName()).log(Level.SEVERE, null, ex);
        }

        return resultados;

    }

    public static ArrayList<String> obtenerIdsRetweets(int retweets) {


        ArrayList<String> resultados = new ArrayList<>();

        ResultSet resultado = conector.Query("SELECT id FROM tweets WHERE retweets > "+retweets);

        try {
            while (resultado.next()) {
                resultados.add(resultado.getString("id"));
            }
        } catch (SQLException ex) {
            Logger.getLogger(InteraccionDB.class.getName()).log(Level.SEVERE, null, ex);
        }

        return resultados;
    }
}

In my case, I prefer to make third class to return structures like ArrayList's or HashTable's making code cleaner at invocation side.

Any suggestion will be appreciated, I care to improve:

  • Exception handling ( I know I'm not doing nothing for it so far)
  • Going for Interfaces
  • Security
  • Java / OOP stuffs
share|improve this question
3  
May be pedantic to say, but I'd really advise against using non-english names (I'm guessing this is Spanish) in code. –  Pierre Arlaud May 6 at 9:32

3 Answers 3

I seen a lot of things what can better so I'm glad you came here to learn.

First :

Your Conectoris a normal class.
The ConectorCliente extends that class and do just calling the super constructor without anything else.
Here is a problem : Or you set your first class abstract if you know you are going to have other implementations (preferable with more code) or your first class should be an interface.

Second :

private Connection  connection  = null;
private Statement   statement   = null;
private ResultSet   set         = null;

String host;
String port;
String login;
String password;
String url;

Why do you do it right for Connection, Statement an Resultset and don't set a modifier before all the strings?
The best practice is make them all private and if you need them outside the class make an getter/setter for it.

Third :

private void Conectar() {

should be :

private void conectar() {

Method names do not start with a capital.
starting with a capital is for classes.

Fourth :

You rely on users of your class that they close your connection.
You will have memory leaks cause they will forget to close your connection.

There are 2 possibilities what you can do.

First :

@Override
protected void finalize() throws Throwable {
    super.finalize();
    cierraConexion();
}

So override the finalize of the class and close the connection there.
This is called when the class is destroyd.
I prefer the second one cause the finalize has issues that it could be that it is never called. As it seems in the comment a nice discussion on this issue :).

Second :

Open and close your connection before you do an action to the DB. Do a try-with-resource or close the connection in the finally block of the try-catch.

Edit : added sample code.

//  Register the driver once in the constructor.

private Connection getConectar() throws SQLException {
    return DriverManager.getConnection(url, login, password);
}

public boolean Update (String update) {
    int result;
    try (Connection connection = getConectar(),
    Statement statement = connection.createStatement()) {
        result = statement.executeUpdate(update);
    } catch (SQLException e) {
        System.out.println("Exception in update method:\n" + e.getMessage());
        return false;
    }
    return (result!=0); // if 0 is returned there is NO row changed => means nothing is updated.
}

Fifth :

Your InteraccionDB has only all static methods and variable.
So no instanciation has to be made.
Why not make the class static and maybe even final when you want to be sure nobody may extend that class.

share|improve this answer
1  
I wouldn't recommend relying on finalize() though... many resources online indicate likewise, e.g. this SO link: stackoverflow.com/a/2506509/2754160 –  h.j.k. May 6 at 7:02
1  
@h.j.k. like I said, personally prefer the second one because I know it has issues. Still a possibility worth mentioning in mine oppinion. –  chillworld May 6 at 7:07
    
Thanks so much, deeply, can you give some code snippets of some of the advices (least 4 which is obvious) –  diegoaguilar May 6 at 7:12
    
Fourth 3: Implement AutoCloseable, and use Class as resource. Very nice review +1 –  Vogel612 May 6 at 7:13
2  
Openings connection is a high-overhead operation, so I don't advise closing it too automatically. Explicit close, with an automatic close in finalize() in case the client forgets, is preferable. –  200_success May 6 at 7:18

Just for starters...

  • Method names in your Conector class should be in camelCase too
  • There is no need for ConectorCliente if all it does is to extend Conector and use the same constructor.
  • Your try-catch across the two classes is either printing out to console directly or creating a new Logger each time. I will suggest creating a static final Logger.getLogger(<class name>) in each class to do logging.
  • Methods' return types should be an interface instead of an implementation, i.e. List<> instead of ArrayList<>
  • I don't think your connection class should be a static field inside InteraccionDB, and in fact I don't think its methods should be all static either... how do you intend to test your code?
  • Too many empty lines, please trim for readability
  • Reduce the try-catch scope inside Conector.Query() Conector.cierraConexion() for Exception to something like SQLException (as you have done in other methods)
share|improve this answer
    
Thanks, about static methods. I find it easier that way, assuming quite any web or even desktop app connection to DB depends upon the DB connection, something failing there and being catched will make the application react. How would this affect testing? –  diegoaguilar May 6 at 5:35
    
For example, changing database connection parameters to test against different databases will imply a code change. One recommended approach is that you can still have a class similar to InteraccionDB, but it gets a database connection instance and can explicitly initialize or close the connection if required. –  h.j.k. May 6 at 5:41
    
I agree about it, however I planned InteraccionDB to be ever rewritten. And assume at it that there will already be a connection going through. –  diegoaguilar May 6 at 5:43

Some things that hasn't been mentioned already:

SQL INJECTION!

Use prepared statements. They are much more secure than normal statements as they prevent SQL injection, which your code currently is vulnerable to. Never use string concatenation to create an SQL statement!


As your ResultSet set is only used inside the query method, I would use it as a local variable inside that method. I would also rename the variable to queryResult.

public ResultSet Query(String query) {
    ResultSet queryResult = null;
    try {
        statement = connection.createStatement();
        queryResult = statement.executeQuery(query);
    }
    catch (Exception e) {
        System.out.println("Exception in query method:\n" + e.getMessage());
    }
    return queryResult;
}

Once the constructor is finished, you're not using your String variables again, pass them on to the connect method and remove them as class fields. Only pass them on as parameters.


public void Desconectar () {
    connection = null;
}

Close the connection before you set it to null! (And rename the method to disconnect)


String or int?

Many of your parameters are Strings. I would use them as ints. A port number is an int, an id for a tweet is (most likely) an int. Don't use them as strings!


Only a suggestion: Use Hibernate (object-relational mapper) and/or C3p0 (connection pooling). They can be difficult to get started with but they are wildly used in many Java projects, for good reasons. Learning them can be both good for your program, and good for your Java knowledge.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.