In programming circles and when talking about code, I’ve often hear mentioned the Perl slogan “There’s more than one way to do it.” Thinking about the many ways we do it, whatever feature ‘it’ represents, I believe that the root cause of many of the software bugs we encounter are because engineers implement the feature in more than one way, then refactor it down the line, then add new conditions, then forgot how it did what it did. This often occurs because programming is like fashion, each season brings about a set of new best practices and technologies. We update the implementation to adapt to the current best practices, new technologies, and new features and don’t remove old bit rotten code.
Recently I a debate with a fellow engineer whose code I reviewed. As it is often the case, we had some existing functionality which we wanted to update with a feature. Most of the code review revolved around one existing method whose existing code looked something like the following.
public void setValue(DataRow dr) throws DataException {
dataHelper.setValue(dr, false, true);
}
For the sake of this example, imagine that DataRow is a map of data that represents a row of data from a database and contains the meta-data of the columns it contains. The new functionality need to support a DataRow with a less number of meta-data, columns, than normally expected. We called this new feature partial value and so it was initially implemented as follows.
public void setValue(DataRow dr) throws DataException {
dataHelper.setValue(dr, false, true);
}
public void setPartialValue(DataRow dr) throws DataException {
setPartial(true);
setValue(qube);
setPartial(false);
}
Believe it or not, I had a very difficult and long drawnout debate with the engineer that implemented the above code. In my mind, there is no debate, the above code can be made to improve. In the engineer’s mind, this was good enough for this feature for the time allotted for the the current specifications for this release and for whatever other reason he counjured up.
One of the concerns I originally brought up with the initial implementation of setPartialValue was that if the setValue method throws an exception after having set the partial state to true, then the code will break out of both methods leaving an improper partial state behind. This will cause problems because the partial boolean field will have an invalid state which will caused problems if you the setValue method is called at a later point. Another problem, with this sort of style of programming, is the following scenario. Imagine that instead of a simple primitive boolean the partial property required an object reference that retained a lot of memory and you had the same exception, the memory for that field might not be garbage collected.
The sort of bugs that can be caused by the above implementation lead me to the following rule of thumb: Avoid instance fields. Always question the need for instance property. Try to be a team player and pass the object reference as a parameter down the stack. If you only need this little bit of state information for a method call, pass it as a parameter!
I couldn’t believe we had this much disagreement over three lines. At that moment I felt that code reviews are another form of micromanagement, but then I remembered who is the one that gets called if there is a bug in the system…. Yeah, me! The approach I was advocating did not require a object instance to keep the state of a boolean, which was only used within the context of the setPartialValue method call. In my approach the set/getPartial methods are not needed and it could have been refactored to the following.
public void setValue(DataRow dr) {
setValue(dr, false);
}
private void setValue(DataRow dr, boolean partial) {
dataHelper.setValue(dr, false, true, partial);
}
public void setPartialValue(DataRow dr) {
setValue(dr, true);
}
The engineer countered that it “does not look nice” aesthetically to have a method call with to many parameters, and that we can’t just add parameters to methods whenever we need to pass another value. Touché. To this I responded that four parameters is not a big deal. Even though four parameter will not negatively affect a method signature, lets imagine that a method signature grew to 10 parameters, this can be solved by encapsulating all these parameters to one object. Programming languages such as Ruby use map literal for optional parameters and long parameter method signatures.
In the end, I won the debate but it didn’t feel like a victory. I doubted myself for pushing so strongly for what I know is right, in the end he was doing all the heavy lifting and this was just one small aspect of the feature as a whole. There is a line that if crossed you will be considered a micromanaging standard fascist, but that if you do not enforce you will inherit unmaintainable code. Coding standards goes beyond naming convention and third party libraries. There is a fine line, and you need to walk the line.