2
\$\begingroup\$

I have this method that lists database objects:

public List<Person> getPersons(int offset, int limit) {
    return jdbcTemplate.query("SELECT * FROM `Person`" + (limit > 0 ? " LIMIT " + (offset > 0 ? offset + "," : "") + limit : "")+" ORDER BY `created` DESC");
}

It looks confusing, so how do I clean up the method?

\$\endgroup\$
3
\$\begingroup\$

If you want to clean up a bit the method, then I think that you should try another, more object oriented way of writing the method (currently the method is written procedurally).

Implementation 1

First lets create few interfaces, which will represent the Limit and the Offset:

public interface Offset {

    public String getOffset();

}

public interface Limit {

    public String getLimit();

}

Next we will need an implementations of this interfaces (I supposed that you are using MySQL as database, since its shorter syntax for limitation and offset is the same):

public MySQLOffset implements Offset {

    private final int offset;

    public MySQLOffset(int offset) {
        this.offset = offset;
    }

    @Override
    public String getOffset() {
        if (offset < 0) {
            throw IllegalArgumentException("Offset couldn't be a negative number.")
        }

        return offset + ",";
    }
}

public MySQLLimit implements Limit {

    private final MySQLOffset offset;
    private final int limit;

    public MySQLLimit(MySQLOffset offset, int limit) {
        this.offset = offset;
        this.limit = limit;
    }

    @Override
    public String getLimit() {
        if (limit <= 0) {
            throw IllegalArgumentException("Limit should be a strictly positive number.");
        }

        return String.format("LIMIT %s %s", offset.getOffset(), limit);
    }
}

Finally we will need one more interface, which will serve us as a glue between this two interfaces (I called it Pagination, since most of the times, offset and limit is used in combination for pagination of some API):

public interface Pagination {

    public String getPagination();

}

public MySQLPagination {

    private final MySQLLimit limit;
    private final MySQLOffset offset;

    public MySQLPagination(MySQLLimit limit) {
        this.limit = limit;
        this.offset = offset;
    }

    public String getPagination() {
        return limit.getLimit();
    }
}

Now, suppose that we have some data access object called PersonDAO, which should retrieve all the persons in a given range (bounded by limit and offset), then the code would look like that (I`ve change the name of getPersons method to retrievePersons, since we actually retrieves the person entries from the database, rather than getting them from some POJO):

.. code ommited ..

    PersonDAO.retrievePersons(new MySQLPagination(new MySQLLimit(new MySQLOffset(offset), limit)));

.. code ommited ..


public List<Person> retrievePersons(Pagination pagination) {
    return jdbcTemplate.query("SELECT * FROM `Person` " + pagination.getPagination());
}

Implementation 2

Suppose that you want to you use the long syntax of limitation and offset. Then the code will look like this:

public interface Offset {

    public String getOffset();

}

public interface Limit {

    public String getLimit();

}

public MySQLOffset implements Offset {

    private final int offset;

    public MySQLOffset(int offset) {
        this.offset = offset;
    }

    @Override
    public String getOffset() {
        if (offset < 0) {
            throw IllegalArgumentException("Offset couldn't be a negative number.")
        }

        return " OFFSET " + offset;
    }
}

public MySQLLimit implements Limit {

    private final int limit;

    public MySQLLimit(int limit) {
        this.limit = limit;
    }

    @Override
    public String getLimit() {
        if (limit <= 0) {
            throw IllegalArgumentException("Limit should be a strictly positive number.");
        }

        return "LIMIT " + limit;
    }
}

public interface Pagination {

    public String getPagination();

}

public MySQLPagination {

    private final MySQLLimit limit;
    private final MySQLOffset offset;

    public MySQLPagination(MySQLLimit limit, MySQLOffset offset) {
        this.limit = limit;
        this.offset = offset;
    }

    public String getPagination() {
        return limit.getLimit() + offset.getOffset();
    }
}

.. code ommited ..

    PersonDAO.retrievePersons(new MySQLPagination(new MySQLLimit(limit), new MySQLOffset(offset)));

.. code ommited ..


public List<Person> retrievePersons(Pagination pagination) {
    return jdbcTemplate.query("SELECT * FROM `Person` " + pagination.getPagination());
}

If you want you could also move the creation of the Pagination object into some factory method like this one:

public Pagination createPagination(int limit, int offset) {
   return new MySQLPagination(new MySQLLimit(limit), new MySQLOffset(offset))
}

Which also comes with some benefits, like more testable code etc.

Benefits of this implementation:

  • Implemented in OOP way
  • The validation of the input parameters is moved into the relevant classes, so the single responsibility principle is respected.
  • Easier to change the implementation
  • Easier for testing (suppose that you use one database for production and one for testing and they have different syntaxis for the limitation and the offset, then you should only mock the createPagination method and everything will work)
  • There is no if/else (or ternary operator) clauses in the getPersons method
  • Also now the getPersons method doesn't know how the limit and the offset of the query is constructed, which is the right way, since it`s work is to retrieve the entries, not to know how to construct them (which means little responsibilities for getPersons method)
\$\endgroup\$
1
\$\begingroup\$

You can create the query one piece at a time using a StringBuilder:

public List<Person> getPersons(int offset, int limit) {
    StringBuilder sql = new StringBuilder();
    sql.append("SELECT * FROM Person ");

    if (limit > 0) {
        sql.append("LIMIT ");

        if (offset > 0) {
            sql.append(offset);
            sql.append(", ");
        }

        sql.append(limit);
        sql.append(" ");
    }

    sql.append("ORDER BY created DESC");

    return jdbcTemplate.query(sql.toString());
}

Or:

public List<Person> getPersons(int offset, int limit) {
    String query = getQuery(offset, limit);

    return jdbcTemplate.query(query);
} 

private String getQuery(int offset, int limit) {
    StringBuilder query = new StringBuilder(75);
    query.append("SELECT * FROM Person ");

    if (limit > 0) {
        query.append("LIMIT ");

        if (offset > 0) {
            query.append(offset);
            query.append(", ");
        }

        query.append(limit);
        query.append(" ");
    }

    query.append("ORDER BY created DESC");

    return query.toString();
}
\$\endgroup\$
1
\$\begingroup\$

I would do something similar to Alessandro's implementation but take advantage of the fact that you can chain calls to append since they also return StringBuilder

public List<Person> getPersons(int offset, int limit) {
    return jdbcTemplate.query(buildQuery(offset, limit));
}

Option 1, with chaining.

private String buildQuery(int offset, int limit) {
        StringBuilder queryBuilder = new StringBuilder("SELECT * FROM PERSON ");
        if (limit > 0) {
            queryBuilder.append("LIMIT ");

            if (offset > 0) {
                queryBuilder.append(offset).append(", ");
            }
            queryBuilder.apepnd(limit).append(" ");
        }

        queryBuilder.append("ORDER BY created DESC");
        return queryBuilder.toString();
    }

Option 2, with ternary.

More compact with ternary kindred to original implementation:

private String buildQuery(int offset, int limit) {
        StringBuilder queryBuilder = new StringBuilder("SELECT * FROM PERSON ");
        if (limit > 0) {
            queryBuilder.append("LIMIT ")
                .append(offset >  0 ? String.format("%d, ", offset) : " ")
                .append(limit).append(" ");
        }

        queryBuilder.append("ORDER BY created DESC");
        return queryBuilder.toString();
    }

Option 3, just the limits

Actually, speaking of string format since only the limit is the variable we can change it to only evaluate for that.

public List<Person> getPersons(int offset, int limit) {
        return jdbcTemplate.query(
            String.format("SELECT * FROM PERSON %s ORDER BY created DESC",
                querylimit(offset, limit)
            )
        );
    }

    private String querylimit(int offset, int limit) {
        StringBuilder limitBuilder = new StringBuilder();
        if (limit > 0) {
            limitBuilder.append("LIMIT")
                .append(offset >  0 ? String.format(" %d, ", offset) : " ")
                .append(limit);
        }
        return limitBuilder.toString();
    }
\$\endgroup\$

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.