Home > Uncategorized > Don’t handle just another state if you can query, please!

Don’t handle just another state if you can query, please!

How often do you find yourself having trouble to understand a big chunk of code? At the moment I’m pretty busy with refactoring just another controller. Even though the number of WTF/min from my office is quite large at the moment, I’m not angry with the fellows who wrote it. The code was made under time pressure and as far as I’m informed, people were under constant threat to be criticized for introducing a bug by changing working code for no apparent reason. Unfortunately refactoring and code cleanups are sometimes hard to sell as “apparent reason”, especially to non-technical staff. As a result code quality starts to decay, changes take longer than necessary and refactoring gets harder every day. I’m placing my bet on my “new kid on the block” credits to go for the risk of breaking code at the moment. So what’s making my life hard at the moment? State! To make the code easier to understand and enhance I’m working hard to eliminate any unnecessary state, because state is what makes it hard to follow a program flow. When writing code you should avoid unnecessary state it in the first place, you’ll make your life easier, trust me!
Let me show you what I mean:

public class Foo{

 private String from;
 private String to;
 private Date when;
 private boolean valid;

 void setFrom(String x){
  if(x == null || x.isEmpty()){
   from="";
   valid=false;
  }else{
   from=x;
   valid=true;
  }
 }

 void setTo(String x){
  if(x == null || x.isEmpty() || x.equals(from)){
   to="";
   valid=false;
  }else{
   to=x;
  }
 }

 void setDate(Date x){
  if(x.after(new Date())){
   when=null;
   valid=false;
  }else{
   when=x;
   setFrom(from);
   setTo(to);
  }
 }

 boolean isValid(){
  return valid;
 }
}

The original code included some UI-widgets, asynchronous web service calls, more sophisticated validation, but it roughly boils down to something like the code you can see above. This example is fairly small, but he ugliest part of it is the “valid” state, which gets updated on any change.
Code like this is harder to follow than necessary, because you have to track the state-change, especially if you try to change the code. Now just compare that to this code:

public class Foo{

 private String from;
 private String to;
 private Date when;

 void setFrom(String x){
  if(x == null || x.isEmpty()){
   from="";
  }else{
   from=x;
  }
 }

 void setTo(String x){
  if(x == null || x.isEmpty() || x.equals(from)){
   to="";
  }else{
   to=x;
  }
 }

 void setDate(Date x){
  if(x.after(new Date())){
   when=null;
  }else{
   when=x;
  }
 }

 boolean isValid(){
  return !from.isEmpty() && !to.isEmpty() 
    && when != null && !when.after(new Date());
 }
}

Just by removing the extra “valid” state and replacing it with a query method, the code got dramatically more readable, made it possible to remove some ugly code in the “setDate” method, which existed solely to manage that field correctly, new refactorings get apparent (like removing the “when=null” branch), plus it is absolutely obvious when this object’s state is considered valid and when it’s not.
This simple example shows a basic rule: Each modified field in your class represents state and each state which can be substituted by a query is usually unnecessary and should be avoided, unless it causes an unbearable performance-penalty.

Update:
I just found another example, which has nothing to do with queries, but in general it’s the same problem with unnecessary state:

class Bar{
 private List<Object> items = new ArrayList<Object>();
 private boolean save;

 public void reset(){
  items.clear();
  save = true;
 }

 public void buzz(int x){
  for(int t = 0; t<x; ++t){
   items.add(SomeFactory.create(x,t));
  }
  if(save){
   save();
  }
 }

 public void bell(int x){
  save = false;
  buzz(x);
  save = true;
  for(int t = 0; t<x; ++t){
   items.add(SomeOtherFactory.create(x,t));
  }
  save();
 }

 private void save(){
  for(Object item : items){
   SomeService.save(item);
  }
 }
}

Believe it or not, but I just had to work on code structured exactly like that, only it had 3 times as many public methods like that. I can hardly express how much I hate it! First it is easy to get wrong, because the callee always has to reset(); before using any of the public method. Second, this class has way too much state and is hard to follow. Third: I didn’t mention that this is a shared class in a multithreaded context. The callee must make sure no two calls use the object at the same time. Please, please with sugar topping, please: Beware of writing code like that! In this case pass the list as a parameters, make the boolean field a parameter and replace it with a meaningful enum. If you want to keep the public API clean, introduce new public methods without these parameters. And what you’ll get is this:

class Bar{
 private enum Save{REQUESTED, SKIPPED}

 public void buzz(int x){
  buzz(x, new List(), Save.REQUESTED);
 }

 public void bell(int x){
  bell(x, new List());
 }

 private void buzz(int x, List items, Save save){
  for(int t = 0; t<x; ++t){
   items.add(SomeFactory.create(x,t));
  }
  if(save == REQUESTED){
   save(items);
  }
 }

 private void bell(int x, List items){
  buzz(x, items, Save.SKIPPED);
  for(int t = 0; t<x; ++t){
   items.add(SomeOtherFactory.create(x,t));
  }
  save(items);
 }

 private void save(List<Object> items){
  for(Object item : items){
   SomeService.save(item);
  }
 }
}

This may be more code, but it is easier to refactor, the public API is hard to use in a wrong way (and has less methods by the way), the code is easy to follow because you don’t have to keep global variables in mind and any object of this class is safe to share.
Let me repeat the mantra once again: Avoid unnecessary state if you can.

Just to keep you interested in case this blog might have been too trivial in your opinion: My next Blog will contain some cool reflection magic.

Advertisements
Categories: Uncategorized Tags: ,
  1. javafinanceguy
    April 16, 2010 at 16:01

    I somewhat agree with this.
    i do think having another state just to check object validity is a bad thing. but it creates a burden on the on the caller to call that method. not calling isValid could create potential problems at a later stage in the process.

    In real sense, if an object, during state change, becomes invalid it should throw a illegalargument exception, right in setTo method.
    but if given an option between throwing/catching illegalargument exception and calling explicitly isValid, i would go with isValid.

    i would recommend having Foo class implement an interface [like ValidateState with isValid method] and then override isValid() in Foo. That way you can pass this object around and someone in the lifecycle of the request can directly handle validity of all objects. [e.g. in jsf request/response lifecycle, when request parameters are applied, it does data validations like numeric, date check. right after that process, you can have jsf call isValid method on objects that implement ValidateState interface. ]

  2. April 17, 2010 at 01:08

    Actually, when I wrote that blog, my intent was not so much focussed on validation, but rather on introducing some boolean or let it be some integer, which represents information which can also be gained by querying other variables or constants. This “valid”-boolean was rather meant as an example for redundant information, which needs to be kept in sync. So you need to take care of it if you change things, you need to take care of it if you want to understand the code. So getting rid of that redundant information usually makes you life easier.
    But you bring up an interesting topic: when to validate and how to fail. I might write my thoughts about that topic one day, but at the moment I’ve got something else in the queue. Stay tuned!

  1. No trackbacks yet.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: