Reducing the complexity of the code

Question

I'm stuck right now with some really weird classes, that have the logic mixed up. Here is the example of the code that generates a query to the database:

if(realTraffic.getPvkp() != null) {
   //Admission point
   if(BeanUtils.isGuidEntity(realTraffic.getPvkp())) {
      findParameters +=
         " and (" + staticTableName() + ".guidPvkp = '" + realTraffic.getPvkp().getGuid()
         + "' or (" + staticTableName() + ".guidPvkpOut = '" + realTraffic.getPvkp().getGuid()
         + "' and " + staticTableName() + ".requestType = " + RequestBean.TRANSIT_TYPE
         + ")";
      if (companyType == CompanyBean.PP_TYPE && !realTraffic.isSkipOther()) {
         // TODO - add non-formed
         findParameters += " or (" + staticTableName() + ".guidPvkpOut is null "
         + " and " + staticTableName() + ".requestType = " + RequestBean.TRANSIT_TYPE
         + ")";
      }
      findParameters += ") ";
   } else {
     // Territorial department
      if(BeanUtils.isGuidEntity(realTraffic.getPvkp().getTerritorialDepartment())) {
         findParameters +=
            " and (Pvkp.guidTerritorialDepartment = '" + realTraffic.getPvkp().getTerritorialDepartment().getGuid()
            + "' or Pvkp.guidFtsDepartment = '" + realTraffic.getPvkp().getTerritorialDepartment().getGuid()
            + "' ) ";
      }
   }
}

This is just a part of a huge set of complex checks I have in method. The question is - how to deal with such code - it has lots of nested if's and checks. What are the common approaches in order to make this code simpler and more elegant?

UPD: I understand how to avoid such code, when writing a new project, but what to do with the existing legacy code?

Answer 1

A good guide to handle such things is in the book from Uncle Bob, called "Clean Code". In your case I'd say:

• put the string concatenations into a method (and use StringBuilder)
• convert an else { if (condition) } to an else if (condition)
• consider to put the companyType == CompanyBean.PP_TYPE && !realTraffic.isSkipOther() in a separate method, since it appears to be some kind of business logic, which might be clearer for the reader if being put in a method called if (isCompanySkippedOver(companyType, realTraffic)

• consider to invert if(realTraffic.getPvkp() != null) to

  if(realTraffic.getPvkp() == null) {return;}
  

to reduce block indentation.

Answer 2

I wouldn't like seeing all that string concatenation for generating SQL queries on the fly. You probably have a countable set, even if it's large. I'd make them static final Strings and use PreparedStatement and bound variables. Your way is too error prone and may risk SQL injection.

I'd be keeping that code in an interface-based persistence/repository/DAO class. I'd think about making it polymorphic so I could pick out the version based on parameters passed.

It it's really complex, think about a state machine or decision tree or decision table. Those can be good ways to tame complexity.

It can be argued that OOP was about getting rid of complex logic like this. See if you can use polymorphism and encapsulation to eliminate it.