Saturday, July 6, 2013

Let's Retire the Flag

OK, 4th of July is over and it's time to put away the flags. No, I don't mean the Stars & Stripes. I'm talking about using flags in your code.
How do you end up with so many flags in your code? Well you begin innocently enough, you have a controller method that calls to a service and returns a list, so you code up something like this:



@RequestMapping(value = "/person/list/", method = RequestMethod.GET)  
      public String loadList(final Model model) {   
           final Iterable<Person> people = service.findAll();  
           model.addAttribute("personList", people);
    return "list"; 
      }  

Then surprise, the requirements change (You knew that was going to happen, didn't you?). So now we need to also filter the list. OK, no problem, we'll pass a parameter in and filter the list.
      
@RequestMapping(value = "/person/list/", method = RequestMethod.GET)  
     public String loadList(@RequestParam final String filter, final Model model) {   
           final Iterable<Person> people = service.findAll();  
           if(StringUtils.isNotEmpty(filter)){  
                //some sort of filtering here
           }  
           model.addAttribute("personList", people);  
           return "list";  
      }  

But you know what happens next, a new requirement. This time there are different types of filters, so now we start to use our flag variable
      
@RequestMapping(value = "/person/list/", method = RequestMethod.GET)  
     public String loadList(@RequestParam final String filter, final Model model) {  
           final Iterable<Person> people = service.findAll();  
           if(StringUtils.isNotEmpty(filter)){ 
                if("filterA".equals(filter){ 
                    //some sort of filtering here
                }else{
                   //some differnt filtering logic
                }
           }
           model.addAttribute("personList", people);  
           return "list";  
      }  

But now what happens if the filters are additive, like in a search. The nested ifs and the procedural code becomes really hard to maintain. Your methods become large and complex and changes become hard to make. As we add more flags, the combinations make it harder and harder to code. Our tests become hard to write, and "the other developers" you know the ones, copy and paste code, skimp on the the test, etc, etc. Your code starts to break under the pressure until nobody wants to touch the code. You might have seen code like this, you might have even written some yourself to just get something done. So what do we do? Well we have to refactor to have smaller methods that do one thing. By changing the parameter to a path variable we can use the URL to take care of the filter param. We break out our methods and eliminate one level of if statements.
  
@RequestMapping(value = "/person/list/{filter}", method = RequestMethod.GET)  
     public String loadList(@PathVariable final String filter, final Model model) {   
          final Iterable<Person> people = service.findAll();  
          if("filterA".equals(filter){ 
            //some sort of filtering here
          }else{
            //some differnt filtering logic
          }
          return list(model, people);
}      
@RequestMapping(value = "/person/list/", method = RequestMethod.GET)  
      public String loadList(final Model model) {   
          final Iterable<Person> people = service.findAll();  ;
   return list(model, people); 
      }  

private String list(final Model model, final Iterable<Person>) {
  model.addAttribute("personList", people);
  return "list";
 }

We now have a simple private method that just displays the list to the view. We should break our methods up further, such as breaking out the filtering into another method. Depending on how complex this is, we could make use of Apache commons and Predicate classes to abstract our filtering logic into a private method. Note that converting from iterable to a list and back again might not be ideal and we should probably create a service method that gives us what we need. In fact, we might also push the filtering logic back into the service layer, if so we already have the URL mappings and it would be a simple refactoring.
  
private Iterator filter(final  Iterable<Person> people, String filter) {
    Predicate predicate=...some sort of Predicate Factory here....
    Collection<Person> peopleList = new ArrayList<Person>();
    CollectionUtils.addAll(peopleList, (Iterator) people);
    CollectionUtils.filter(peopleList, predicate);
    return peopleList.iterator();
}   

After the refactoring we have much more flexibility, we could multiple filterings on a list or pass the result to a sort routine. Our methods now are small and granular, although not very useful, they are easy to read and understand. In order to make them useful we need to composite them with other methods and expose them as part of the API. This might be URLs or it could be a formal interface such as the service layer API. If we keep the methods small and specific it makes our code so much easier to read and should allow us to respond to change so much easier. So the next time you find yourself reaching for the flag, ask yourself if you really need it, or is a bit of refactoring in order.