Skip to main content
Code Review

Return to Revisions

2 of 3
edited body
Simon Forsberg
  • 59.7k
  • 9
  • 157
  • 311

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.

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(" )");

Voilà!

Simon Forsberg
  • 59.7k
  • 9
  • 157
  • 311
default

AltStyle によって変換されたページ (->オリジナル) /