Ken Hansen bio photo

Ken Hansen

Ken is a software engineer and leader of software engineers who enjoys the collective work of Raymond Smullyan and knows that solving a problem is only half the battle.

Email

Mathematics is the art of giving the same name to different things.
— Henri Poincare

The other day I ran across the following in a pull request:

public interface UsefulObjectDAO {
	public UsefulObject  accessEntry(int primaryKey, int secondaryKey);
}

Comment	: personally, I would prefer just "getEntry" vs "accessEntry".
Seems to imply you are, in fact, accessing a cache.

This got me thinking.

I personally would prefer to not use “get” because I feel that “get” has a lot of connotation to it. “Get” implies getter (and possibly setter) functionality. Getter methods in java are often boilerplate code, which causes people to make other assumptions. This reminded me of an issue that happened earlier in my career that should have been easy to find but wasn’t.

The standard boilerplate code for a getter and setter generally looks like this:

public class UsefulObject {
	
	private float cost;
	
	public float getCost() {
		return cost;
	}
	
	public void setCost(float cost) {
		this.cost = cost;
	}
	...
}

The cost defaults to zero if not set, otherwise the cost is whatever was set. Would you write a unit test for this? (Of course not, its boilerplate code.) I usually don’t even write the getters and setters, I just have the IDE generate them. You can even set the code coverage tools to ignore getters and setters when calculating out how much coverage there is.

Now someone had previously written the following code:

for (UsefulObject usefulObject : usefulObjects) {
	// only process valid usefulObjects
	if(usefulObject.getCost() > 0.0f){
		process(usefulObject);
	}
}

This code greatly reduced the number of UsefulObject that had to be processed because while there were reasons for the objects to exist, only a small number of the UsefulObject instances had a cost and were used at any point in time.

The cost field of a UsefulObject now had an additional connotation. The application worked fine for several months but then one day the application stopped working. Initial investigations showed it had a significant increase in CPU usage. The change that caused this situation was well intentioned and was done an attempt to fix something else:

public class UsefulObject {

	private float cost;


	public UsefulObject(float cost) {
		this.cost = cost;
	}

	public float getCost() {
		// There is no such thing as a free lunch!
		if (cost <= 0.0f) {
			return 0.01f;
		}
		return cost;
	}

	public void setCost(float cost) {
		this.cost = cost;
	}
	...
}

This change did not impact any of the existing unit tests as no one had written unit tests for the getters and setters (but then again who does?). Everyone was thinking of and talking about the objects in preconceived terms. They were looking at the recent code changes in the application but unfortunately UsefulObject was declared and defined in a separate library. It wasn’t until preconceived ideas were thrown out that the performance issue was fixed.

Names and labels often have meanings beyond what they are directly stating. There was quite an argument over what the actual bug was. Some felt that using the value of the cost field to determine which objects to process was the cause. Their suggestion was to create a new field to directly indicate whether to process or not process the object. Others felt that changing cost to always return a value was incorrect. They wanted to add another method to either return cost or 0.01f if cost was less than or equal to zero.

The real cause of the problem was the connotation around the cost field. By explicitly stating how fields are used would have prevented any misunderstanding:

public class UsefulObject {

    // Initialized to a value not otherwise used
    private float cost = Float.NEGATIVE_INFINITY;

    public float getCost() {
	return cost;
    }

    public void setCost(float cost) {
	this.cost = cost;
    }

    /**
     * Returns cost if it has been set otherwise returns alternate value.
     * Used whenever cost must have a value (i.e. displayed)
     * @param alternate value to return if cost has not been set
     * @return float which is either the cost or the alternate value. 
     */
    public float getCostOrElse(float alternate) {
	if(Float.isInfinite(cost)) {
	    return alternate;
	}
	return cost;
    }

    /**
     * Helper function to check if cost has been set and this instance should be processed. 
     * @return true if the cost has been set; otherwise false
     */
    public boolean shouldProcess() {
	return !(Float.isInfinite(cost));
    } 
    ...
}

By adding a new method, the UsefulObject class encapsulates how to determine if it will be processed. Adding additional methods has some advantages over adding a new field which will act as a flag. First, adding a method to a class does not take up additional memory, which could cause a problem in applications which are memory sensitive. Secondly, if an application serializes the object - if the objects are stored for later use or are logging - adding a flag would require additional code for versioning and backwards compatibility.Finally, it will be easier to change the code in the future if the business rules around which UsefulObject instances to process should need to change.

Changing a getters expected behavior often causes bugs but not changing the getter often leads to code that repeats itself:

	UsefulObject usefulObject = ...
	float costVal = usefulObject.getCost();
	if(costVal <= 0.0f) {
		costVal = 1.0f;
	}
	doSomthingWithCost(costVal);
	...

If the call to getCost is in one place, then the above approach is acceptable. However, if it is in many places, then this breaks the DRY (Don’t Repeat Yourself) principle. I suggest leaving the getter alone and adding a second method - getCostOrElse - to access the value or return a default value:

	private static float MINIMAL_COST_VALUE = 1.0f;
	...
	UsefulObject usefulObject = ...
	doSomthingWithCost(usefulObject.getCostOrElse(MINIMAL_COST_VALUE));
	...

This approach is much more readable and easier to adjust than checking the value returned by the getter each time. Take this into consideration, what would happen if zero became a valid cost? When you are developing code, it is easy to change the code. But once the applications has been released into the wild, it often becomes a lot harder. The time and effort to code review and test the change becomes a function of the size of the change.

Strive for clarity in your objects and methods. Your objects and methods should have a well defined purpose. This improves the readability and maintainability of your code. You will be thankful the next time you have to go back and change the code.