Clean Code irk

Say you have something like the following in your code:


boolean typeFolding = false;

 /**
  * Gets the status of type folding. 
  */
  public boolean getTypeFolding() {
    return this.typeFolding;
  }

 /**
  * Sets the status of type folding. 
  * @param typeFolding true or false if type folding should be enabled or not.
  */
  public void setTypeFolding(boolean typeFolding) {
    this.typeFolding = typeFolding;
  }

The above is pretty basic and is probably used even in your own code. While the above works, a more meaningful API could be something like the following.


boolean typeFolding = false;

 /**
  * Enable type folding. 
  */
  public void enableTypeFolding() {
    this.typeFolding = true;
  }

 /**
  *Disable type folding. 
  */
  public void disableTypeFolding() {
    this.typeFolding = false;
  }

 /**
  * Is type folding currently enabled
  */
  public boolean isTypeFoldingEnabled() {
      return this.typeFolding;
  }

Yes the two are equivalent in functionality, but the big difference comes when you try to use the API and have to maintain code that uses it.


if (getTypeFolding()) {
   // Do something that turns it off
   setTypeFolding(false);
else
  // Do something that turns it on
   setTypeFolding(true);

Compared to the following:


if (isTypeFoldingEnabled())
   disableTypeFolding();
else
   enableTypeFolding();

The later is pretty straight forward to read, the former while it works, leaves a lot open to interpretation and reliance on JavaDoc to tell you the intent.

What prompted this post was some similar getter and setter code I ran across in the Turmeric Eclipse Plugins.

Advertisements
This entry was posted in clean code, craftsmanship, eclipse, java, turmeric. Bookmark the permalink.

9 Responses to Clean Code irk

  1. Jeff McAffer says:

    While I don’t disagree with the sentiment, the first solution would be more simply stated as
    setTypeFolding(!getTypeFolding)
    No need for if’s etc.

    More generally however I’ve found that the use of *boolean* flags like this to be error prone and evolutionarily limiting. What happens when you want partial folding? (e.g, tri-state setup). Many times we have put in place a boolean setting only to find someone wanting more than two states. The latter approach is less fun than say using bitmasks to enable testing of multiple states at once.

  2. Jim Przybylinski says:

    The problem I see with enable/disable methods over set style methods is it is difficult to pass down a parameter and just plug it in. I agree that get/set is usually poor for boolean parameters, but in this case, I would say uses/use is a better style. Then the example code can become

    if(this.usesTypeFolding())
    this.useTypeFolding(false);
    else
    this.useTypeFolding(true);

    or even better

    //invert typeFolding
    this.useTypeFolding(!this.usesTypeFolding());

    • kingargyle says:

      I try to avoid boolean parameters. Why force a parameter on it when it isn’t necessary?

      the use is a bit better than the set/get method, but it still has the ucky feeling of the boolean parameter. Ideally I like to have zero parameters on a method for a class.

  3. Ian Bull says:

    Well I agree that the last snippet is much better, there was a small sleight of hand. In your first use of the API you make the assumption that setting typeFolding to false is not enough to disable type folding, so you added a comment ‘do something that turns if off’. However, in the better ^H^H^H^H^H^H second example, you don’t include this comment.

    Maybe this was an oversight, or maybe that’s the point. We could roll the ‘turning off of type folding’ into the method disableTypeFolding() since that’s its name. Of course doing that will put a dependency from disableTypeFolding to whatever mechanism we are using to actually accomplish this task. Is that desired?

    The other problem you could have with the second approach is consistency. If your system currently uses setSyntaxHighlighting, setLineNumbers, setXXX, and you decide to use enable/disableTypeFolding, then you make it more challenging to use. I strongly believe that it’s better to be consistency poor than inconsistency good when it comes to API.

    • kingargyle says:

      Ian, it was intentional. The method name should make it clear what the intent is. There are times to use set/get and there are times not to do it. The problem is when you get lots of different people over the years working on some code.

      I would favor on correcting the API to make it more useable and deprecating the old API that was less clear on how and what is to be doing.

      I also agree with Jeff. Avoid the the use of boolean parameters.

  4. How about setTypeFolding(isTypeFoldingEnabled()) to toggle.

  5. Pingback: Dave Carver: Clean Code irk

  6. Prakash G.R. says:

    You got the variable name wrong in the first example. It shouldn’t be typeFolding, rather typeFoldingEnabled, which gives us:

    	private boolean typeFoldingEnabled;
    	
    	public boolean isTypeFoldingEnabled() {
    		return typeFoldingEnabled;
    	}
    
    	public void setTypeFoldingEnabled(boolean enabled) {
    		this.typeFoldingEnabled = enabled;
    	}
    

    Which gives us the usage of:

    	
    	if (isTypeFoldingEnabled())
    		setTypeFoldingEnabled(true);
    	else
    		setTypeFoldingEnabled(false);
    

    or even better, as per the earlier comments:

    	setTypeFoldingEnabled(!isTypeFoldingEnabled());
    • kingargyle says:

      Actually, no that is the way the original code was setup. I agree the variable name should be typeFoldingEnabled. That is another topic on variable naming standards though.

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