QualityDev.net

No! This isn’t a blog about Code Reviews. It’s actually a book review of Bob Martin’s ‘Clean Code’. However the amount of code to read in the book may seem like a series of code reviews (more on that in a bit).

Each chapter contains a different subject to enhance code quality.
From the simple practices that are often overlooked, for example; naming conventions and formatting styles, through the classic tried and tested; Kent Beck’s 4 Simple Rules of Design (Run all tests, contain no duplication, code to express intent and minimize the number of classes and methods); to some gems that can take years to uncover; such as bad practices for returning null (with the correct idiom) and good practices for testing threaded code.
This reviewer certainly chuckled guiltily when reading some of the examples of code that could be written better (”Oh yeah, I remember doing that once!!”).

I was surprised to see a few reviews on other blog sites and amazon about the fact this was not written solely by Bob but by different authors at Object Mentor. I actually thought this was a good thing as each chapter is a separate concern and the writing styles differ so the reader can be engaged differently from one chapter to the next. Plus, there is a lot of Java and the change in temperament clears ones head after reading all that code.
This book does not follow a specific ‘project’ throughout, like many other books today, but concentrates on real world examples to illustrate the points made. This is not a book one can read quickly if the reader wants to real get involved and practice what this book teaches.

One may presume that my opening paragraph was negative on the amount of code in this book. On the contrary, I found this to be an excellent model to get, mainly inexperienced [Java], programmers to look at more code written by experienced experts and take a look at the internals of classes in popular frameworks such as JUnit and JCommon.

Chapter 17 is a concise description of each of the smells and heuristics used throughout the book and if you think you have your coding practices down to a fine art, read through this - you never know where you can learn a new thing or two. Also, if you are still one of the few out there that are not performing Unit Tests (shame on you) then just the first four pages of chapter 9 (p 121 - 124) should sell it to you, and your manager if required.

Now the not so good! Maybe it’s because we are so much into ‘best’ practices that a few things niggled me.
There are a few contradictions in the book. Not only where the advise to never leave commented out code in your application is ignored, but in some cases (e.g. p66) some Catch blocks with no code contain a comment as to why they are empty, while some are indeed completely empty (no comment) - we believe this is not a good practice and is one of the few cases where a comment is indeed useful in a block of code.

I also found a paragraph of slight Utopia-ism (is that a real word?). It is mentioned that programmers should not just get the code working but successfully refine working code before they move onto the next piece of functionality. Unfortunately I know of no customer who would be willing to pay for someone to sit and refactor code when it is functionally working correctly. This may sound contradictory on a Code Quality site but i think it is the real world. I think a better way of arguing this point is to define what is meant when a piece of functionality is ‘done’. This would include the refinement (and documentation which is usually not evident) - maybe I’m just arguing semantics?

In summary this book is a great resource of areas of good coding practices that each chapter has had many whole books dedicated to. I would like to see this book in Computer Science undergraduate courses as I feel to get these practices in early is only a good thing. For any professional, the information contained here in one book gives huge value to the reader.

Lawrence Oliva touched on a great issue when using metrics in an organization “…it is important for metrics to support your project and not just be a bottomless pit for data collection and weekly presentation.”

I agree with this sentiment. Far too often do we feel good about collecting relevant metrics and then not finding time to implement the data in a feedback loop to increase the awareness and knowledge in the organization. This is the key for effective use of metrics to increase the quality of the next iteration.

Oliva continues “If currently used metrics are not driving your team’s time, energy, and skill set mix towards achieving project success, it’s time to select a new set of measurements.”

Selecting new metrics may be premature if the current ones are not being used effectively. If problems are arising after the metrics have been fed back to the relevant people, including additional explanation as why anomalies were apparent and suggestion for improvement, then a change in metrics may be required.

Oliva suggests selecting two metrics for the Project Manager, two for the Development team and two more for the Management Team.
I would enhance this to three metrics each, totaling nine. There are several reasons for this but the main one is that having two reports will often lead people to implicitly feel that a choice needs to be made to improve one or the other, whereas often they are not mutually exclusive. I Adapt Rothman and Derby’s “Rule of 3″. One solution is a trap with no choice, two alternatives give a choice of this or that while three alternatives leads people to come up with more options.

I like the metrics chosen for PMs and Senior Management however one of Oliva’s suggested metrics for the development team, Lines Of Code (LOC), I would argue is not even a metric. Even if we agreed it was, what does a a sudden drop in this number mean - a section of the codebase was deleted and lost (bad) or someone refactored the codebase to improve quality (good)?

My three initial metrics for the development team would be:
Bug Count (also stated by Oliva)
Blocks of duplicate code - various tools can do this and can be implemented as part of a build. If a bug exists in duplicated code on one instance may be found and corrected leading to the false conclusion that a particular bug has been fixed - it’s only been fixed in that part of the code.
Static Code Analysis Warnings - again various tools do this and once configured correctly gives huge benefit in identifying possible bugs pre-release

I do understand that other metrics are just as important - Code Coverage for example. But I had to choose only three

Earlier this week on Javalobby, I posted an extract from our monthly newsletter regarding our analysis of the ‘missing braces in If Statement’ rule firing and the potential bug involved.

If you omit braces and use Static Analysis tools it is a problem actually finding these bugs. Why? Because you probably have the rule turned off!

Having any tool tell you of violations in your code purely due to a stylistic preference will result in thousands of false positives and eventually demotivate the developer from using the tool, therefore, for the other good causes these tools promote, in this case it is probably better to switch the rule off.

Hopefully Unit Tests are in place to cover these areas if static analysis is not used. The only other way to find these bugs is manual code review (laborious, time consuming and introduces human error i.e. the bug may be missed anyway) or debugger tracing.

Another interesting aspect about the post was the comments. Many of those who omit braces in If Statements do so for readability purposes. Psychologically, this may mean that these developers see readability as a higher aspect of quality than possible fault-prone code. I’m not stating that readability is not important, far from it. However from a business perspective, having the code released with less faults is a higher quality perspective.

One evening after sessions at the Better Software Conference, Dan North and I got into a discussion regarding the phrase ‘Best Practices’ and concluded that this term was actually a misnomer.

Let’s take a non-software analogy; wearing a seat belt in a moving vehicle.

With all the studies that have been performed over the years, one may believe that wearing a seat belt is a best practice. However, for a very small minority of cases, it is not the best thing to be wearing a seat belt. EMS and Police personnel in some countries are not required to wear seat belts, because they can respond faster unhindered by a seat belt. Also some drivers, like those in the TV show Ice Truckers, do not wear seat belts because they need to be able to jump out of their rig the moment they hear ice beginning to crack. These may be minority cases and for over 99% of us it is a good practice (never mind a legal requirement) to wear a seat belt–but it is not a Best Practice.

When we talk about Best Practices we really mean ‘the current best thing to do in a particular context’. When coding, depending on the situation, we try to solve an issue using some practice we know works effectively. Wait! Haven’t we heard this before? Isn’t this what we use to describe Patterns? On reflection, patterns would be a sub-set of what we wish to achieve and still other ‘good practices’ will need to be enforced somehow, especially early in a developers career.

The terms ‘Best Practices’ and ‘Standards’ are also used interchangeably, especially when applied to code. This is wrong! Coding standards may be formed from current ‘best practices’ but other issues such as law and external environmental concerns may mean that the code standards are not necessarily the current best way to write a piece of code. 

This is just one example of the terminology problems that needs to be clarified in our profession. I don’t believe a committee can enforce this, but terminology, over time, will become more widely used and accepted as our industry matures, as has occurred in other industries over a period of time.

Many have written about language ambiguity being one of the key issues which leads to misunderstanding requirements and that a language needs to evolve so business and IT understand each other.  I would go one step further and declare that at least two formalized languages are needed. One for the Development team for lower level issues so statements such as ‘this lends itself to a Strategy Pattern’ or ‘favor composition over inheritance’ are understood by all the team members and another for Business Analysts, Project Leaders and Stakeholders to ensure no ambiguity exists in the requirements.

IMO the first language is closer to being realized than the second. Maybe interest in UML will be rejuvenated with Microsoft’s recent announcement of UML support to be added to Visual Studio Team System providing another step towards this goal.

We are commonly taught to create objects using the JavaBean pattern, where for every variable there is a corresponding getter and setter, or by passing in attributes to the object’s constructor on creation. For example:

Employee emp = new Employee("name", "some address", "city", "state",
             employee_number, salary, department, … more arguments);

Not only is this not ideal to read, you must remember to pass the values in the correct order. Even type safety will not help you if, for example, you interchange the city and state arguments. Also, if the Employee class has only two mandatory fields (name and employee number) the developer must add null entries to fields they do not want to set at creation time.

This method also violates the Open/Closed principle (PDF link) where classes should be open for extension but closed for modification – thus to increase functionality, add new code (via abstraction), rather than changing old code.

Using a Builder Pattern allows you create the same object in the following way:

Employee emp = new Employee.Builder("Rich Sharpe", 32)
            .address("1 Java Way")
            .city("Javaland")
            .salary(12000.00)
            .department(AccountsDept)
            .build();

Not only is this more robust than the JavaBean pattern or the long unwieldy constructor, but the creation of this object is now far more elegant to read.

Here is the partial Employee class:

public class Employee {
  private String name;
  private String address;
  private long employee_number;
  private double salary;

  public static class Builder {
    private String name;
    private String address;
    private long employee_number;
    private double salary;

    public Builder(String name, int employee_number){
      this.employee_number = employee_number;
      this.name = name;
    }

    public Builder salary(double salary){
      this.salary = salary;
      return this;
    }

    public Builder address(String address){
      this.address = address;
      return this;
    }

    //Additional setters here...
    //Finally add the build method

    public Employee build(){
      return new Employee(this);
    }
  }

  private Employee(Builder builder){
    this.name = builder.name;
    this.address = builder.address;
    this.employee_number = builder.employee_number;
    this.salary = builder.salary;

    //Copy remaining data from Builder to Employee...
  }

}

Create a static ‘builder’ class within your class and provide individual setter methods. Finally add a ‘build’ method that returns an instance of its class as a parameter to a private constructor of the class you wish to build.

If a specific exception needs to be thrown, then the private Employee constructor and public build() methods can handle this (remember IllegalArgumentException and IllegalStateException do not have to be specifically declared in a throws clause).

Usually this type of creation pattern is used when an object will not change during its life. I chose to create a User class as for this example as a huge benefit of this type of creation is that the object is immutable and therefore thread safe.

Although a user may change some attributes; such as their last name if married or address when moving, these are so rare that it may be better to just delete the current object and create another as object creation is very inexpensive and one retains the benefits of an immutable object.

Recently, I was fortunate enough to catch Josh Bloch’s ‘Java Puzzlers Episode VI: The Phantom-Reference Menace/Attack of the Clone/Revenge of the Shift’. One puzzle in particular concerned the use of threading in JUnit testing. This is a great example of a problem developers may face, however, I believe it also points to a much more significant problem – false positives in feedback systems.

Reproduced here (with the author’s permission) is the problem:

Q. How Often Does This Test Pass?

The assumed answer may be a) or b) due to race conditions when the thread accesses the variable ‘number’. The actual answer is c) It always passes. This is because the assertion is running in a background thread to the ‘test’ method and JUnit does not support concurrency and is unaware of any action or exception thrown in the background thread, and so will exit with success. The code to resolve the issue is included at the bottom of this post.

Once you know what the problem is, it’s not that difficult to resolve. But therein lies the problem. JUnit passes this test every time. A manual code review would, in most cases, probably not catch this and, in any case, how many passing tests are even reviewed? This test will pass, the code will be shipped and it’s not until a run time problem occurs that the problem is reported back and the scratching of heads occurs as all the unit tests pass. (OK – I’ve taken a bit of a liberty here, one would hope system testing would catch this but it is not always the case).

The only really effective way to catch problems like these is through the use of static code analysis tools. FindBugs is one tool that can find and flag this particular problem. These tools are very good but not a panacea; of course some problems may still escape undetected. However, the extensibility of these code analysis tools ensures that, once discovered, rules covering these problematic areas can be added, enhancing the tool’s power and utility.

(For those interested, take a look at Puzzler number 5 – ‘Mind the Gap’ (PDF link), in which the java.io.skip(gap_to_skip) returns any value between 0 and the parameter passed in. One would wrongly assume that the return value would be that of the next element after ‘gap_to_skip. Bloch stated that in the source code of the JDK, there are 67 instances of this call, and in 56 cases the return value is not checked!)

My take from all of this? Although false positives are a fact of life, we can and should use static analysis to find those cases that can be found, and we should be diligent about adding new rules, where possible, to cover problems that crop up, so that they don’t happen again.

Solution:

Bloch showed a couple of possible solutions to this problem:

1. Create instance fields in the test to hold any exceptions generated in the background thread and set them in the thread’s run method.

volatile Exception exception;
volatile Error error;

Thread t = new Thread(new Runnable() {
public void run(){
try{
assertEquals(2, number);
} catch(Error e){
error = e;
} catch(Exception e){
exception = e;
}
}
});


2. Check those fields in a tearDown method.

//Triggers test case failure if any thread asserts failed
public void teardown() throws Exception{

if(error != null)
throw error;
if(exception != null)
throw exception;
}