Bad Code Got You Down?

Does your code look like the muck on the left ("Before")? Does it contain poorly formatted code, long and hard-to-follow methods? You might even think the code on the left (derived from an actual production system) is fine the way it is. It might be the par for most shops, but you can do better.

The "before" code is typical. Its long methods hide all sorts of insidious system duplication. Identifier names are terse and often unhelpful. Comments are either a waste of time or just plain wrong. Variables exist that have no use (groupType). Chunks of horrid logic (switchReturnValues) require patient and painstaking step-throughs.

The code on the right ("After") takes advantage of the fact that there were hundreds of similar rules in the (defunct) system. By applying tests and adhering to careful refactoring strategies, I was able to reduce the CheckStateRule class to what you see here. The remainder of its code ended up either gone or refactored into common, closed abstractions. (Some of it went into BaseRule.) The duplicate code is gone. Lots more code is out of sight and out of mind. That's lots of code that you should never have to touch again.

Before

import java.util.*;
import java.io.*;

/**
 *
 * Check if the state the case is located in
 * is equal to argument passed in.
 *
 */

public class CheckStateRule extends BaseRule {

   // constants

    private static final String EXCEPT = "Exception: ";
   String errStr;
   boolean missingData;

   protected boolean
    eval(Case c, Vector val, boolean returnTrue) {

   boolean returnVal = false;
   String argVal = null;

   String groupType = null;

   try {
      if (val == null || val.size() != 1) {
          errStr = "Can't convert value list to a state";
         missingData = true;
         return false;
      } else {
         argVal = (String)val.elementAt(0);
      }

      // get the location object off the form and get the
      // state the case resides in
      Location loc = (Location)c.getFormComposite("location");
      if (loc == null) {
         errStr =
      "Cannot determine the state this case is located in.";
         missingData = true;
         return false;
      }
      String st = loc.getAttribute("state");
      String state = (st == null ? null : st.toUpperCase());
      if (state == null) {
         errStr =
      "Cannot determine the state this case is located in.";
         missingData = true;
         return false;
      } else if (state.equals(argVal)) {
         returnVal = true;
      }

      // return value
      Vector returns = switchReturnValues(returnTrue, returnVal,
         "Case does not reside in '" + argVal + "'",
         "Case resides in '" + argVal + "'");
      returnVal = ((Boolean)returns.elementAt(0)).booleanValue();
      errStr = (String) returns.elementAt(1);

   } catch (Exception e) {
       errStr = EXCEPT + e.getMessage();
       missingData = true;
       return false;
   }

   return returnVal;

   }

   public static Vector switchReturnValues(
         boolean returnTrue,
         boolean returnVal,
         String falseErrStr,
         String trueErrStr)
         throws Exception {


         if (!returnTrue)
            returnVal = !returnVal;

         String retErrStr = null;
         if (returnVal == false) {
            if (returnTrue)
               retErrStr = falseErrStr;
            else
               retErrStr = trueErrStr;
         }

         Vector returns = new Vector();
         returns.addElement(new Boolean(returnVal));
         returns.addElement(retErrStr);

         return returns;
      }
}