1

I am developing a toy data access mechanism in Java 6 (and above). Each model class should have a findById static method that should instantiate an object from the row with the specified id. I have come up with the approach displayed below. Is my approach considered good practice? If not, what could be improved?

The database (MySQL) bootstrap script:

create database test;
create user test identified by 'test';
grant all on test.* to test;
use test;
create table products(id integer,name varchar(10));
insert into products values(1,'led');

The source code:

import java.sql.*;

class Test {
    public static void main(String[] args) throws SQLException, ClassNotFoundException {
        Class.forName("com.mysql.jdbc.Driver");
        Product p = Product.findById(1);
        System.out.println(p.id + " " + p.name);
    }
}

class Database {
    static <T extends Model<T>> T findById(T m, String sql, int id) throws SQLException {
        try (Connection conn = DriverManager.getConnection("jdbc:mysql:///test", "test", "test");
                PreparedStatement stmt = conn.prepareStatement(sql)) {
            stmt.setInt(1, id);
            try (ResultSet rs = stmt.executeQuery()) {
                rs.next();
                m.load(rs);
            }
        }
        return m;
    }
}

abstract class Model<T> {
    abstract void load(ResultSet rs) throws SQLException;
}

class Product extends Model<Product> {
    int id;
    String name;

    static Product findById(int id) throws SQLException {
        return Database.findById(new Product(), "select * from products where id=?", id);
    }

    @Override
    void load(ResultSet rs) throws SQLException {
        this.id = rs.getInt("id");
        this.name = rs.getString("name");
    }
}
prekageo
  • 348
  • 3
  • 15

4 Answers4

2

I'd rather use DAO based approach. You'll need to create one GenericDao<T> class with basic CRUD methods, and all derived DAO classes will have CRUD capabilities out of the box for specified entity class.

Here're two articles which demonstrate the described technique: http://www.codeproject.com/Articles/251166/The-Generic-DAO-pattern-in-Java-with-Spring-3-and http://www.ibm.com/developerworks/java/library/j-genericdao/index.html

Roman
  • 59,060
  • 84
  • 230
  • 322
2

You are mixing concerns and responsibility, introducing tight coupling between your entities (Product) and your data-access layer.

You should separate

  • the entities (which only have getters / setters and possibly internal business logic, according to your overall model you may want to separate that too)
  • the data-access layer: I would have interfaces for each of your entities (ProductDao) with the methods you want to execute to retrieve / store / delete your entities. Then you can have concrete implementations of these using your chosen technology (JDBC in your case). So if later you want to change your data access tech, you can have another implementation of these (JdbcProductDao and HibernateProductDao) for example.

You may even want to go one step further, and dissociate the DAO layer from the actual repository layer, but that could be seen as premature optimisation, depending on the number of different entity classes you have in your system.

That has many benefits:

  • Light coupling is better design overall
  • Better testability, etc.

Also, it's not necessarily a good idea to try to have generic methods everywhere: usually you'll find the findById you want is slightly different for each of your entities, and some of them don't fit the generic method you described in Database (I'm not even mentioning it's a static method, which is a bad smell). In my current team, we use the rule of three: introduce refactored generic classes / method only when you're writing the 3rd elements of your system that would benefit from it. Else we consider it premature optimisation.

Guillaume
  • 21,938
  • 14
  • 54
  • 69
  • Thank you very much for your response. In case, I finally decide to make findById generic, I could use reflection. Is there any better way? – prekageo Sep 03 '12 at 13:47
  • Reflection is very slow. If you need good performances for your app, it may kill it. Test it before you actually use it in your code (run the same query 100000 times in a row, measure time) – Guillaume Sep 03 '12 at 15:02
  • Thanks for the suggestion. Actually this code is not going to be used in production environment. The only purpose is to personally investigate the possibility of writing a generic DAO mechanism in Java without using reflection. Do you think that it is possible? – prekageo Sep 03 '12 at 15:06
  • Everything's possible if you put some effort into it :) Is it worth it? Not sure. Think carefully about what you want to do, then do it, and do only what's necessary. Premature optimisation is the biggest time-waster in programming. You'll end up building a very complex system that may not even be used in the future. – Guillaume Sep 04 '12 at 08:45
1

I like the basic design. But for a real production project I would make 3 changes:

  1. in data base and any resource code add a finally block and close each connection (you have only 1) in separate try-catches or you will get con leaks
  2. in Data base: use a shared connection pool
  3. in the Product getById if products are doing to be reused cache them in a HashMap, if already loaded then return that instead if making a new object every time. this depends on usage but we do it for a lot of tables that have 100 to 5,000 rows and are changes occasionally but read many times.
tgkprog
  • 4,405
  • 4
  • 39
  • 66
0

You are reinventing Object Relational Mapping (ORM) and Data Access Object (DAO) methodologies. There exist many Java libraries which do exactly what you are trying to do here, for example Hibernate. I think you may find this library more useful than getting the right answer to this question.

gigadot
  • 8,743
  • 7
  • 33
  • 49
  • I know that I should be using Hibernate! But it is a toy project mostly for experimentation and without any prospect of scaling. I, totally, agree with you that in a production environment I should be using Hibernate. – prekageo Sep 03 '12 at 13:08