6
\$\begingroup\$

I'm trying to make some methods more reusable and have now three classes that work together: AbstractDao (the superclass), MemberDao (extends AbstractDao) and ProjectTaskDao (extendsAbstractDao). The method insert is very similar in both subclasses with some minor differences:

insert method in MemberDao:

public void insert(Member member) throws SQLException {
try (Connection connection = dataSource.getConnection()){
    try(PreparedStatement statement = connection.prepareStatement(
            "INSERT INTO members (member_name, email) values (?, ?)",
            Statement.RETURN_GENERATED_KEYS
    )){
        statement.setString(1, member.getName());
        statement.setString(2, member.getEmail());
        statement.executeUpdate();

        try (ResultSet generatedKeys = statement.getGeneratedKeys()) {
            generatedKeys.next();
            member.setId(generatedKeys.getLong("id"));
        }
    }
}

}

insert method in ProjectTaskDao:

public void insert(ProjectTask task) throws SQLException {
try (Connection connection = dataSource.getConnection()){
    try(PreparedStatement statement = connection.prepareStatement(
            "INSERT INTO project_tasks (task_name) values (?)",
            Statement.RETURN_GENERATED_KEYS
    )){
        statement.setString(1, task.getName());
        statement.executeUpdate();

        try (ResultSet generatedKeys = statement.getGeneratedKeys()) {
            generatedKeys.next();
            task.setId(generatedKeys.getLong("id"));
        }
    }
}

}

Thesee methods do the same with some minor differences with the "statement.setSomething()". I`m not that advanced in Java yet so I'm kind of stuck on this. I would also like to pull these methods in as well:

list method in MemberDao:

public List<Member> list() throws SQLException {
List<Member>  members = new ArrayList<>();
try (Connection connection = dataSource.getConnection()) {
    try (PreparedStatement statement = connection.prepareStatement("SELECT * FROM members")) {
        try (ResultSet rs = statement.executeQuery()) {
            while (rs.next()) {
                Member member = new Member();
                member.setName(rs.getString("member_name"));
                members.add(mapRow(rs));
            }
        }
    }
}
return members;

}

list method in ProjectTaskDao:

public List<ProjectTask> list() throws SQLException {
List<ProjectTask>  tasks = new ArrayList<>();
try (Connection connection = dataSource.getConnection()) {
    try (PreparedStatement statement = connection.prepareStatement("SELECT * FROM project_tasks")) {
        try (ResultSet rs = statement.executeQuery()) {
            while (rs.next()) {
                //ProjectTask projectTask = new ProjectTask();
                //projectTask.setName(rs.getString(""));
                tasks.add(mapRow(rs));
            }
        }
    }
}
return tasks;

}

And here is the whole superclass as it stands currently:

public abstract class AbstractDao<T> {
protected final DataSource dataSource;

public AbstractDao(DataSource dataSource) {
    this.dataSource = dataSource;
}

protected T retrieve(Long id, String sql) throws SQLException {
    try (Connection connection = dataSource.getConnection()) {
        try (PreparedStatement statement = connection.prepareStatement(sql)) {
            statement.setLong(1, id);
            try (ResultSet rs = statement.executeQuery()) {
                if (rs.next()) {
                    return mapRow(rs);
                } else {
                    return null;
                }

            }
        }
    }
}

protected abstract T mapRow(ResultSet rs) throws SQLException;

}

Let me know if you need more information to help! Thanks :D

\$\endgroup\$
  • \$\begingroup\$ Is your code working as you expect it to? \$\endgroup\$ – Aryan Parekh Oct 29 at 15:03
  • \$\begingroup\$ Yeah it works now, but wondering if it is possible to make it so i dont have to type the same method in every new classDao.java i make in the future. \$\endgroup\$ – Petter Haugland Oct 29 at 15:04
  • 1
    \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$ – BCdotWEB Oct 29 at 15:05
  • \$\begingroup\$ @PetterHaugland Sorry about my downvote then (removed it now), I earlier got the impression that you have asked for a review of code that wasn't working. \$\endgroup\$ – Aryan Parekh Oct 29 at 15:09
  • \$\begingroup\$ First time posting here so im dont really know everything here yet ':) \$\endgroup\$ – Petter Haugland Oct 29 at 15:10
3
\$\begingroup\$
  1. The first advise is to not over-architect the classes.

  2. Keep alternatives in mind - JPA for instance eclipseLink. A bit of not reinventing the wheel as the database field has a long history of "clever" code.

    This is an O/R mapping translating data base Relations into java Objects and vice versa. For a beginner I would first refrain from such magic, and continue as you did. (Your mapRow.)

    The entity classes like Member in general are kept small without logic. And for every entity class there is a repository class with data access (retrieve and such). They rely on a persistence manager singleton instance that takes care of the connection per transaction session (time doing some task).

  3. A long ID and using generated keys is used often.

    For the ID inheritance can be used. Too much inheritance can be painful however.

So keep id in its own base class, and do

public class Member extends IdEntity { ... }

public void insert(Member member) throws SQLException {
    try (Connection connection = dataSource.getConnection()) {
        try (PreparedStatement statement = connection.prepareStatement(
                "INSERT INTO members (member_name, email) values (?, ?)",
                Statement.RETURN_GENERATED_KEYS)) {
            statement.setString(1, member.getName());
            statement.setString(2, member.getEmail());
            statement.executeUpdate();

            SQLUtils.fillGeneratedKeys(member, statement);
        }
    }
}

public abstract class SQLUtils {

    public static void fillGeneratedKeys(IdEntity entity, PreparedStatement statement) throws SQLException {
        try (ResultSet generatedKeys = statement.getGeneratedKeys()) {
            generatedKeys.next();
            entity.setId(generatedKeys.getLong("id"));
        }
    }

3a. Similar is the initialisation of a time stamp field. If you disregard the database server's clock and use java's clock, think of a locale independent time.

  1. Consider a "transaction" level; grouping of data base access.

    Several database operations, say some INSERTs and UPDATEs best function under the same connection. Possible as transaction, an error raising a rollback and otherwise the data is committed at the end. This cleans up the repetition of try (Connection ....

In every case consider a connection pool or at least getting the connection by your own utility function.

  1. For query methods do not use null results but Optional or exceptions. It not only prevents NullPointerExceptions, but allows chaining:

Several usage like:

A.retrieve().map(A::getChildren()).map(List::size).orElse(0);

protected Optional<T> retrieve(long id, String sql) throws SQLException {
    try (Connection connection = dataSource.getConnection()) {
        try (PreparedStatement statement = connection.prepareStatement(sql)) {
            statement.setLong(1, id);
            try (ResultSet rs = statement.executeQuery()) {
                if (rs.next()) {
                    return Optional.of(mapRow(rs)=;
                }
                return Optional.empty();
            }
        }
    }
}

Here Long can be long (It is a long id field too). For retrieve-by-id an exception would probably be better.

Here it could be a painful use of inheritance as the child class provides logic; but it is okay.

protected abstract String retrieveByIdSQL();

public final T retrieveById(long id) throws SQLException {
    String sql = retrieveByIdSQL();
    try (Connection connection = dataSource.getConnection()) {
        try (PreparedStatement statement = connection.prepareStatement(sql)) {
            statement.setLong(1, id);
            try (ResultSet rs = statement.executeQuery()) {
                if (rs.next()) {
                    return mapRow(rs);
                }
                throw new SQLException(...);
            }
        }
    }
  1. Consider translating an SQLException to your own child of a RuntimeException, by rethrowing catched exceptions. Checked exceptions like SQLException must be catched, which nowadays with Stream usage can be cumbersome. But you could postpone this for the fare future.
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Wow thanks for your time there! Ill try it out and mark it as answered 😄😄 \$\endgroup\$ – Petter Haugland Oct 30 at 2:16
  • \$\begingroup\$ Thanks alot for the help! I have now figured it out and it works perfectly! :D \$\endgroup\$ – Petter Haugland Oct 30 at 16:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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