7
\$\begingroup\$

I've a method which is used to build a native sql query. I've 4 String builder as input and based on different conditions, I need to build my query.

Here is the code

private void appendConditions(StringBuilder query, StringBuilder condition, StringBuilder condition2,
    StringBuilder condition3) {
if (StringUtils.isNotEmpty(condition.toString())) {
    query.append(ApplicationConstants.AND+"(");
    query.append("( " + condition + " )");
    if (StringUtils.isNotEmpty(condition2.toString())) {
        query.append(ApplicationConstants.AND);
        query.append("( " + condition2 + " )");
        if (StringUtils.isNotEmpty(condition3.toString())) {
            query.append(ApplicationConstants.AND);
            query.append("( " + condition3 + " )");
        }

    } else {
        if (StringUtils.isNotEmpty(condition3.toString())) {
            query.append(ApplicationConstants.AND);
            query.append("( " + condition3 + " )");
        }
    }
    query.append(" )");
} else {
    if (StringUtils.isNotEmpty(condition2.toString())) {
        query.append(ApplicationConstants.AND+"(");
        query.append("( " + condition2 + " )");
        if (StringUtils.isNotEmpty(condition3.toString())) {
            query.append(ApplicationConstants.AND);
            query.append("( " + condition3 + " )");
        }
        query.append(" )");
    } else {
        if (StringUtils.isNotEmpty(condition3.toString())) {
            query.append(ApplicationConstants.AND+"(");
            query.append("( " + condition3 + " )");
            query.append(" )");
        }
    }
}

}

When I run my Sonar report, it says the Cognitive complexity is higher for this method. Can I simplify this methods avoiding many if-else loops

\$\endgroup\$
  • \$\begingroup\$ Where does ApplicationConstants.AND come from? \$\endgroup\$ – Bergi Feb 14 at 5:28
  • 1
    \$\begingroup\$ Do you have unit tests that exercise this code? This is tangential to the question but for code this complex you NEED unit tests before you start changing it. Since its just building strings it should be easy to unit test. \$\endgroup\$ – Freiheit Feb 14 at 14:48
27
\$\begingroup\$

Let's look for some patterns in your code:

First of all, we notice that you have three parameters with similar names. condition, condition2 and condition3. These names indicates a code smell, that we should use an array or list instead.

Taking one step at a time, we see this code is repeated multiple times:

if (StringUtils.isNotEmpty(condition.toString())) {
    query.append(ApplicationConstants.AND+"(");
    query.append("( " + condition + " )");

if (StringUtils.isNotEmpty(condition2.toString())) {
    query.append(ApplicationConstants.AND);
    query.append("( " + condition2 + " )");

if (StringUtils.isNotEmpty(condition3.toString())) {
    query.append(ApplicationConstants.AND);
    query.append("( " + condition3 + " )");

if (StringUtils.isNotEmpty(condition3.toString())) {
    query.append(ApplicationConstants.AND);
    query.append("( " + condition3 + " )");

if (StringUtils.isNotEmpty(condition2.toString())) {
    query.append(ApplicationConstants.AND+"(");
    query.append("( " + condition2 + " )");

if (StringUtils.isNotEmpty(condition3.toString())) {
    query.append(ApplicationConstants.AND);
    query.append("( " + condition3 + " )");

if (StringUtils.isNotEmpty(condition3.toString())) {
    query.append(ApplicationConstants.AND+"(");
    query.append("( " + condition3 + " )");

This indicates that we can extract a method. (We can also notice that the "(" should be added for the first true if-statement, this is important but we will ignore this for now and get back to this later).

private boolean addParameter(StringBuilder query, StringBuilder condition) {
    if (StringUtils.isNotEmpty(condition.toString())) {
        query.append(ApplicationConstants.AND);
        query.append("( " + condition + " )");
        return true;
    }
    return false;
}

private void appendConditions(StringBuilder query, StringBuilder condition, StringBuilder condition2,
    StringBuilder condition3) {
if (addParameter(condition)) {
    if (addParameter(condition2)) {
        if (addParameter(condition3)) {
            // we don't actually care about the result here
        }
    } else {
        if (addParameter(condition3)) {
            // not here either
        }
    }
    query.append(" )");
} else {
    if (addParameter(condition2)) {
        if (addParameter(condition3)) {
            // again, don't care about result
        }
        query.append(" )");
    } else {
        if (addParameter(condition3)) {
            // yeah ok, here we actually care...
            query.append(" )");
        }
    }
}

Now it's pretty clear that query.append(" )"); should always be done last, so that can be moved out and then we can avoid some if statements.

private void appendConditions(StringBuilder query, StringBuilder condition, StringBuilder condition2,
    StringBuilder condition3) {
if (addParameter(condition)) {
    if (addParameter(condition2)) {
        addParameter(condition3);
    } else {
        addParameter(condition3);
    }
} else {
    if (addParameter(condition2)) {
        addParameter(condition3);
    } else {
        addParameter(condition3);
    }
}
query.append(" )");

And now we're doing the same in both some if's and else's, so that can be simplified:

private void appendConditions(StringBuilder query, StringBuilder condition, StringBuilder condition2,
    StringBuilder condition3) {
if (addParameter(condition)) {
    addParameter(condition2);
    addParameter(condition3);
} else {
    addParameter(condition2);
    addParameter(condition3);
}
query.append(" )");

Once again we're doing the same thing:

private void appendConditions(StringBuilder query, StringBuilder condition, StringBuilder condition2,
    StringBuilder condition3) {
addParameter(condition);
addParameter(condition2);
addParameter(condition3);
query.append(" )");

So we're essentially doing the same thing three times. This is where a loop would be handy.

private void appendConditions(StringBuilder query, StringBuilder... conditions) {
    for (StringBuilder condition : conditions) {
        addParameter(condition);
    }
    query.append(" )");

And now of course, we don't really need that separate addParameter method.

private void appendConditions(StringBuilder query, StringBuilder... conditions) {
    for (StringBuilder condition : conditions) {
        if (StringUtils.isNotEmpty(condition.toString())) {
            query.append(ApplicationConstants.AND);
            query.append("( " + condition + " )");
        }
    }
    query.append(" )");

Now, let's get back to the parenthesis: +"(" is added in the first true if-statement in your original code. So this, in connection with the closing " )" can be handled by for example keeping a boolean to check for if an if-statement has been true so far.

private void appendConditions(StringBuilder query, StringBuilder... conditions) {
    boolean conditionUsed = false;
    for (StringBuilder condition : conditions) {
        if (StringUtils.isNotEmpty(condition.toString())) {
            query.append(ApplicationConstants.AND);
            if (!conditionUsed) {
                conditionUsed = true;
                query.append(" (");
            }
            query.append("( " + condition + " )");
        }
    }
    if (conditionUsed) {
        query.append(" )");
    }

But when reflecting more about what it is that you do, we see that you have a collection of StringBuilders, you check each if it is not empty, then you join them together with ApplicationConstants.AND.

    String specialConditions = Arrays.stream(conditions)
        .map(StringBuilder::toString)
        .filter(StringUtils::isNotEmpty)
        .collect(Collectors.joining(ApplicationConstants.AND));
    if (StringUtils.isNotEmpty(specialConditions)) {
        query.append(" (");
        query.append(specialConditions);
        query.append(" )");
    }

Voilà!

| improve this answer | |
\$\endgroup\$
8
\$\begingroup\$

A very short review;

  • Escape your parameters with escapeSql, this is so important!
  • You should pass the conditions in array, so that the call can pass an arbitrary amount of conditions
  • ApplicationConstants.AND <- total overkill, just use AND
| improve this answer | |
\$\endgroup\$
  • 9
    \$\begingroup\$ Going down the SQL path, I'd argue that parameterized queries is even better than escaping the parameters. \$\endgroup\$ – Simon Forsberg Feb 12 at 18:23
  • \$\begingroup\$ Call me OCD crazy, but I do both (escape, then pass to a parameterized query.) Heck, when I get my way, I put everything in stored procedures (DMLs and DML templates are never exposed in code, nor in config files), called with CallableStatements and with parameters already escaped. It's extra work to set up, but it has saved me countless hours in maintaining and trouble shooting. \$\endgroup\$ – luis.espinal Feb 13 at 18:07
  • 4
    \$\begingroup\$ @luis.espinal escape and then parameterize? If this means what I think it means the end effect will be that you're storing an SQL-escaped version of the data in the database, which is an absolute nightmare to anyone trying to maintain it after you \$\endgroup\$ – DreamConspiracy Feb 14 at 16:06
  • \$\begingroup\$ This is only a concern if the data has questionable "injections". A string with "John Doe" can be sql-escaped ad infinitum yielding "John Doe." I do this when I have to interface with teams or software that might take values I send and concatenate them into SQL statements (rather than using binding variables.) I've had more headaches by cowboys that do that than by doing what I've just described. YMMV, depending on how you (the generic you) codes and what kind of teams you must interact with. \$\endgroup\$ – luis.espinal Feb 15 at 21:49
  • \$\begingroup\$ This should probably go to chat, but using parameterized queries and escaping is just bad practice, in all cases. \$\endgroup\$ – konijn Feb 18 at 13:00
7
\$\begingroup\$

I wouldn't follow this approach at all, trying to create a dynamic SQL by hand using StringBuilder or StringBuffer.

It is inevitable that the complexity will shoot off the roof. And it is very rare to find a need for building SQL statements dynamically like that.

In 30 years, I haven't seen a justification. Every application I've known has a finite set of access patterns, ergo a finite set of SQL statements it needs.

Instead, I would suggest you use parameterized SQL statements (see example below from this tutorial)

String sql = "update people set firstname=? , lastname=? where id=?";

PreparedStatement preparedStatement =
        connection.prepareStatement(sql);

preparedStatement.setString(1, "Gary"); 
preparedStatement.setString(2, "Larson"); 
preparedStatement.setLong  (3, 123);

int rowsAffected = preparedStatement.executeUpdate();

Better yet, load that string from a resource bundle or property.

Other possibilities include using a DSL like jOOQ (example below, from the jOOQ's site):

create.select(AUTHOR.FIRST_NAME, AUTHOR.LAST_NAME, count())
      .from(AUTHOR)
      .join(BOOK).on(AUTHOR.ID.equal(BOOK.AUTHOR_ID))
      .where(BOOK.LANGUAGE.eq("DE"))
      .and(BOOK.PUBLISHED.gt(date("2008-01-01")))
      .groupBy(AUTHOR.FIRST_NAME, AUTHOR.LAST_NAME)
      .having(count().gt卌)
      .orderBy(AUTHOR.LAST_NAME.asc().nullsFirst())
      .limit(2)
      .offset(1)

Or QueryDSL (example below):

QCustomer customer = new QCustomer("c"); // alias for the CUSTOMER table

SQLTemplates dialect = new HSQLDBTemplates(); // SQL-dialect
SQLQuery query = new SQLQueryImpl(connection, dialect); 
List<String> lastNames = query.from(customer)
    .where(customer.firstName.eq("Bob"))
    .list(customer.lastName);

Or, if you are using JPA, to use the JPA's Criteria API (example below):

Subquery<Department> subquery = criteriaQuery.subquery(Department.class);
Root<Department> dept = subquery.from(Department.class);
subquery.select(dept)
  .distinct(true)
  .where(criteriaBuilder.like(dept.get("name"), "%" + searchKey + "%"));

criteriaQuery.select(emp)
  .where(criteriaBuilder.in(emp.get("department")).value(subquery));

Good luck.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ So much this. If you absolutely need to build a query at runtime, use something like the Criteria API with for example Hibernate \$\endgroup\$ – MTilsted Feb 14 at 18:24
2
\$\begingroup\$

I am not sure what exactly you're trying to achieve, but after reading through the code I have following thoughts:

  1. Why do you use StringUtils.isNotEmpty instead of .isEmpty()? It looks fairly unlikely that condition.toString() would return null. Also, for StringBuilder you may want to use length (condition.length != 0)
  2. Since if's logic for different conditions is almost identical, you can use a loop:
private void appendConditions(StringBuilder query, StringBuilder... conditions) {
  StringBuilder subQuery = new StringBuilder();
  for (StringBuilder condition: conditions) {
    if (condition.length != 0) {
      if (subQuery.length != 0) {
        query.append(ApplicationConstants.AND);
      }
      subQuery.append("( " + condition + " )");
    }
  if (subQuery.length != 0) {
    query.append(ApplicationConstants.AND+"("+subQuery+")");
  }
  }
}
| improve this answer | |
\$\endgroup\$
  • 6
    \$\begingroup\$ != "" should not be used when comparing strings. It would compare the object reference instead of the actual content. \$\endgroup\$ – Simon Forsberg Feb 12 at 18:24
  • \$\begingroup\$ @SimonForsberg sure, stupid me. It should have been .isEmpty(). Fixed! \$\endgroup\$ – Alexander Pushkarev Feb 16 at 13:10

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.