I visit a lot of different companies and see a lot of absolutely ghastly code. What I was looking at here wasn’t all that bad. Variable names were not chosen to limit keystrokes and method names appeared to be descriptive. This was good news, as I needed to understand how this was put together before I could offer help with a testing strategy.

As we walked through the code, I noticed that there were classes in the project directory ending in ‘Test’. This took me by surprise. Usually when I’m helping people with legacy code issues, there aren’t any tests. Here, there were tests in place and they actually passed. Very cool, but now my mission wasn’t clear to me as I thought my help was needed getting tests in place around legacy code.

The developers clarified that they wanted help in testing private methods. Ah ha, the plot thickens.

The question of testing private methods comes up frequently whether I’m teaching a class or consulting on a project. My first response is a question. “Is the private method being tested through the public interface to the class?” If that’s the case, then there’s nothing to worry about and I can steer the conversation away from testing private methods to testing behaviors of a class instead of trying to test individual methods. Note that a private method being tested through its public interface would be guaranteed if the class was developed TDD style where the test is written first, followed by one or more public methods to make the test pass, followed by one or more extract method refactorings, which would be the birth of the private methods. This is almost never the case. My client didn’t know how the code was developed, but by inspection they concluded that the parameters of the test were adequately exercising the private method.

It looked like my work was done here. But not so fast.

I have a policy that says that whenever I have code up in an editor, I have to try to leave it just a little bit better than when I found it. Since we had put the testing matter to rest and we still had some time left in the conference room before another meeting started, I suggested that we see if we could make some small improvements to the code we were examining.

As I said earlier, the code wasn’t horrible. The code smell going past my nostrils was Long Method and the cure was Extract Method.

The overall structure of the method we were examining was

    
    if( conditional_1 )
    {
        // do lots of complicated stuff
    }
    else if( conditional_2 )
    {
        // do even more complicated stuff
    }
    else
    {
        // do stuff so complicated nobody understood it
    }
    

where conditional_1 was some horribly convoluted expression involving lots of &&’s, ||’s, and parentheses. Same for condition_2, which also had a few ^’s thrown in for good luck. To understand what the method did, one would have to first understand the details of how the method did it.

I asked the developers if they could come up with a nice, descriptive method name that described what I’m calling condition_1 so that we could do an extract method refactoring and the code would look like:

    
    if( descriptive_name() )
    {
        // do lots of complicated stuff
    }
    // etc
    

Now there were less details to understand when trying to determine what this method did. If we were to stop here and check in the code, we could leave the session feeling good as the code is better than when we started. But we still had time before we had to abandon our conference room so I pressed on.

“Can you summarize what this code does as a descriptive method name,” I asked. The developers pondered a few moments and came up with what they felt was a good name. Excellent. We did the same procedure for the “else if” clause. When we finished that, one of the developers said something along the lines of, “That was the easy part, I have no idea what this [else] remaining code does.” I was going to pat everybody on the back and call it a day because the code had improved tremendously from when we started, but the developers seemed to have a “we can’t stop now” attitude. They studied the code, pondered, cursed, discussed some more, and then one of them said, “This code can never execute!”

I’d like to use the expression, “You could have heard a pin drop,” to describe the silence in the room, but since there were only three of us, the phrase looses its power. As it turns out, now that the if() and else if() conditionals were given descriptive names and people could grok them at a glance, it became obvious that the business rules didn’t permit the final else – the first two conditions were all that could exist and the most complicated code of all was never being called. This was downright comical!

I asked if the tests would still pass if we deleted the code and after a short examination of the tests, the developers weren’t as confident that the test parameters actually hit that area of code. There was a log() statement in that code and one of the developers was going to examine the production logs to see if the code ever executed.

So there you have it, refactor your code and the bad parts just go away!