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+"("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(" )");
}
}
}
private void appendConditions(StringBuilder query, StringBuilder... conditions) {
for (StringBuilder condition : conditions) {
if (StringUtils.isNotEmpty(condition.toString())) {
query.append(ApplicationConstants.AND+"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(" )");
}
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(" )");
}
}
}
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(" )");
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(" )");
}
}
}
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(" )");
}
ViolàVoilà!
Violà!
Voilà!
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(" )");
Violà!