Data Access Layer and Business Objects

Question

This is my first cut at separating the data access layer out of my very old spaghetti codebase.

I have tried to keep this as simple as possible, it is for a small project, so I am not really interested in learning Spring or Hibernate or other framework, and I don't mind writing a bit of SQL.

I basically have a 1-to-1 relation of database table to business object.

This is the Data Access Layer

package dataaccesslayer;
import dbutils.handlers.IgnoreUnderscoreBeanHandler;
import dbutils.handlers.IgnoreUnderscoreBeanListHandler;
import racemanager.businessobjects.Progression;
import org.apache.commons.dbutils.QueryRunner;
import shared.SLDbHelper;
import java.sql.SQLException;
import java.util.List;
public class DalProgression {
 private static final String SQL_SELECT_ONE =
 " SELECT * "
 + " FROM progressions "
 + " WHERE id=? ";
 private static final String SQL_SELECT_ALL =
 " SELECT * "
 + " FROM progressions "
 + " ORDER BY name ASC ";
 public static Progression selectOne(int id) {
 QueryRunner qr;
 Progression progression;
 try {
 qr = new QueryRunner(SLDbHelper.getInstance().getDataSource());
 progression = qr.query(SQL_SELECT_ONE, new IgnoreUnderscoreBeanHandler<Progression>(Progression.class), id);
 } catch (SQLException ex) {
 throw new RuntimeException(ex);
 }
 return progression;
 }
 public static List<Progression> selectAll() {
 QueryRunner qr;
 List<Progression> progressions;
 try {
 qr = new QueryRunner(SLDbHelper.getInstance().getDataSource());
 progressions = qr.query(SQL_SELECT_ALL, new IgnoreUnderscoreBeanListHandler<Progression>(Progression.class));
 } catch (SQLException ex) {
 throw new RuntimeException(ex);
 }
 return progressions;
 }
}

This is the related Business Object

package businessobjects;
import java.util.Date;
public class Progression {
 private int id;
 private String name;
 private Date modified;
 private Date created;
 public int getId() {
 return id;
 }
 public void setId(int id) {
 this.id = id;
 }
 public String getName() {
 return name;
 }
 public void setName(String name) {
 this.name = name;
 }
 public Date getModified() {
 return modified;
 }
 public void setModified(Date modified) {
 this.modified = modified;
 }
 public Date getCreated() {
 return created;
 }
 public void setCreated(Date created) {
 this.created = created;
 }
 @Override
 public String toString() {
 return this.getName();
 }
}

Usage example

int id = 10;
Progression p = DalProgression.selectOne(id);
List<Progressions> pList = DalProgression.selectAll();

1. Fire away and shoot holes in my code, so I can improve it.

2. One thing I am not sure about is when I do joins should I have a separate business object for the result, and as such as corresponding data access layer class to perform those queries?

Answer 1

How small is small? Hibernate, JPA and so on would help you get rid of a lot of boilerplate code.

That said.

1. I'd change selectOne to selectById, similarly for other querying functions, which I'm sure you'll get a lot of, or in general find a good, consistent naming scheme for those functions.

2. Keep in mind that int has a particular size if you already have a large legacy database.

3. If possible remove all the statics and make DalProgression a real object (so you can set and reuse QueryRunner/SLDHelper and create better tests).
4. Did I mention tests. (Legacy == ) please write tests. It honestly helps so much.
5. In order to do that get rid of the SLDHelper singleton,
6. which is not a good name, since it has both an unknown acronym in it (at least to me, otherwise you'd have to explain the name) and additionally Helper, which can also mean everything (the acronym goes for DalProgression as well, less so since you've mentioned Data Access Layer; still I'd say something like ProgressionRepository, ProgressionStore or so would be better). If possible I'd just avoid such names.
7. Move the exception conversion into a separate class so you can reuse that, even if you have to create a wrapper for the QueryRunner.
8. You could also just directly return from inside the try block and initialise the variables with their values directly, i.e. try { QueryRunner qr = ...; return qr.query(...); } catch ....
9. I'd also create a SQLRuntimeException or so in order to catch exactly those later instead of grepping for RuntimeException.
10. Have a plan for transactions in your API, unless you already have, that is.
11. IgnoreUnderscoreBeanHandler is already generic, there are options (e.g. via reflection) to get rid of the class literal argument.
12. Unless you have a specific use for that particular toString method I wouldn't use that, rather have something to identify the object in a stacktrace, let's say, the id ("<Progression id=42 name=whatever>").
13. Thinking about it, if you have the ID and the timestamps in all business objects, create an abstract base class or an interface to capture that notion.

For the joins, I guess that unless you have extra data in join tables, creating separate business objects isn't necessary. So I'd imagine it would be easier to understand if you just have methods List<Foo> selectFooForProgression(Progression) or Map<Foo, Progression> selectFoosForProgressions(List<Progression>) in the DalProgression class, basically where you start the join from.

• bumperbox: So for point 3 & 5. You would expect the usage to look like this DalProgression dal = new DalProgression(dataSource); dal.selectById(id)
• bumperbox: Can you please elaborate on point 6, I don't understand what you mean by "a really opaque name"
• ferada: Yes, I mean my point of view is oriented along the lines of Spring/DI, so even if you don't use that, I'd rather construct those objects in one place instead of having static methods. I'm updating point 6/7.