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.

Answer 3

Looks like the code I have to work on every day...

If I come across a big bunch of if's like this I make a judgement call on whether I think the conditions will change in the future and how much I need to increase the clarity of the code.

If the body of the if will change but the condition won't I have taken to trying to factor out the condition to a method that returns a boolean. That method then gets unit tested to within an inch of it's life. I realize this isn't an ideal refactoring but it is a pragmatic solution that increases code clarity in that section. It's probably worth point out that the code I'm talk about here doesn't have any unit tests at the moment so wide scale refactoring is difficult - I'm guessing you are in a similar position.

Answer 4

Start by writing characterization tests, that is, unit tests that will fix the current behaviour of the system. You need at least 4 tests to cover all possible paths.

Only then, when you are protected by the tests, you can refactor.

How to refactor depends on your technical skills and inclinations. Being an OOP guy, I would probably push towards an object that encapsulates the incremental building of the where condition:

condition.add("foo = ?", x);
condition.add("bar <> ?", y);
... 
condition.toSql(); // returns the sql code
condition.getObjects(); // returns a list of x, y...

Other approaches may work.

The key idea is to separate abstraction levels; at one level you have the business rules for selecting stuff from the database; at a much more concrete level you have string concatenations. You don't want to mix these levels.