public void perform(String operatorName) {
      BigDecimal op1 = stack.pop();

      if ("+".equals(operatorName)) {
         BigDecimal op2 = stack.pop();
         stack.push(op1.add(op2));
         currentMode = Mode.inserting;
      } else if ("-".equals(operatorName)) {
         BigDecimal op2 = stack.pop();
         stack.push(op2.subtract(op1));
         currentMode = Mode.inserting;
      } else if ("!".equals(operatorName)) {
         op1 = op1.round(MathContext.UNLIMITED);
         BigDecimal result = BigDecimal.ONE;
         while (op1.compareTo(BigDecimal.ONE) > 0) {
            result = result.multiply(op1);
            op1 = op1.subtract(BigDecimal.ONE);
         }
         stack.push(result);
      } else {
         throw new MathOperatorNotFoundException();
      }
   }

Unlike the last example, I’ll provide the entire class. Feel free to make changes to this class as well. However, for now focus on the perform(...) method.

One note, Philip Schwarz recommended a change to what I proposed to avoid the command/query separation violation. I applied his recommendation before posting this updated version.

The Whole Class

package com.scrippsnetworks.calculator;

import java.math.BigDecimal;
import java.math.MathContext;

public class RpnCalculator {
   private OperandStack stack = new OperandStack();
   private Mode currentMode = Mode.accumulating;

   enum Mode {
      accumulating, replacing, inserting
   };

   public RpnCalculator() {
   }

   public void take(BigDecimal value) {
      if (currentMode == Mode.accumulating)
         value = determineNewTop(stack.pop(), value);

      if (currentMode == Mode.replacing)
         stack.pop();

      stack.push(value);
      currentMode = Mode.accumulating;
   }

   private BigDecimal determineNewTop(BigDecimal currentTop, BigDecimal value) {
      BigDecimal newTopValue = currentTop;
      String digits = value.toString();
      while (digits.length() > 0) {
         newTopValue = newTopValue.multiply(BigDecimal.TEN);
         newTopValue = newTopValue.add(new BigDecimal(Integer.parseInt(digits
               .substring(0, 1))));
         digits = digits.substring(1);
      }

      return newTopValue;
   }

   public void enter() {
      stack.dup();
      currentMode = Mode.replacing;
   }

   public void perform(String operatorName) {
      BigDecimal op1 = stack.pop();

      if ("+".equals(operatorName)) {
         BigDecimal op2 = stack.pop();
         stack.push(op1.add(op2));
         currentMode = Mode.inserting;
      } else if ("-".equals(operatorName)) {
         BigDecimal op2 = stack.pop();
         stack.push(op2.subtract(op1));
         currentMode = Mode.inserting;
      } else if ("!".equals(operatorName)) {
         op1 = op1.round(MathContext.UNLIMITED);

         BigDecimal result = BigDecimal.ONE;
         while (op1.compareTo(BigDecimal.ONE) > 0) {
            result = result.multiply(op1);
            op1 = op1.subtract(BigDecimal.ONE);
         }
         stack.push(result);
      } else {
         throw new MathOperatorNotFoundException();
      }
   }

   public BigDecimal getX() {
      return stack.x();
   }

   public BigDecimal getY() {
      return stack.y();
   }

   public BigDecimal getZ() {
      return stack.z();
   }

   public BigDecimal getT() {
      return stack.t();
   }
}

Today we picked one method in particular and attempted to work our way through it. This method was a classic legacy method (no unit tests). It also had a switch on type and then it also did one or more things based on a set of options. All of this was in one method.

If you read Fowler’s Refactoring Book, it mentions a combination of encapsulating the type code followed by replacing switch with polymorphism for the first problem in this method (the switch). We were able to skip encapsulating the type code since we wanted to keep the external API unchanged (legacy code).

So we first created a base strategy for the switch and then several empty derived classes, one for each of the enums. This is a safe legacy refactoring because it only involved adding new code.

Next, we created a factory to create the correct strategy based on the type code and added that to the existing method (we also added a few virtual methods). Again, a safe refactoring since it only involved adding effectively unused code (we did create the factory using nearly strict TDD). Finally, we delegated from the original method to the strategy returned from the factory. Safe again, since we had tested the factory.

So far, so good. But next, we wanted to push the method down to each of the subclasses and remove the parts of the logic that did not apply to each given type. We did a spike refactoring to see what that’d be like and it was at least a little dicey. We finally decided to get the original method under test so that as we refactored, we had the safety net necessary to refactor with confidence.

We started by simply calling the method with null’s and 0 values. We worked our way through the method, adding hand-rolled test doubles until we came across our first static class.

• Added an instance of the class as a static member (essentially creating a singleton).
• Added a property setter and getter (making it an overridable singleton).
• We then copied the body of the static method into an instance method, which we made virtual.
• We then delegated the static method to the virtual method.
• Then, in the unit test, we set the singleton to a hand-coded test double in the setup and reset the singleton in the tear down method.

We had to do this several times and on the third time (I think it was the third time), the hand-rolled test double would have had to implement several (17ish) methods and it became clear that we were ready to use a mocking framework. They are using Moq so we started using Moq to accomplish the remainder of the mocking.

After some time, we managed to get a test that essentially sent a tracer bullet through one path of the method we wanted to get under test. When the test turned green there was much rejoicing.

• This method currently makes calls to the service layers and those calls depend on both an enumeration (replaced with a shallow and wide hierarchy of strategies) and options (to be replaced with a composition of strategies).
• It also changes some values in an underling domain object.

So that’s what we needed to characterize.

• Begin as we already had, get through the method with a tracer.
• Determine the paths we want to get under test.
• For each path:
  • Create a test using strict mocks (which fail as soon as an unexpected method is called)
  • Use a Setup to document this call as expected – this is essentially one of the assertions for the characterization test.
  • Continue until we have all the Setups required to get through the test.
  • Add any final assertions based on state-based checks and call VerifyAll on the Moq-based mock object.

This would be a way we could work through the method and characterize it before we start refactoring it in earnest.

This might sound like a lot of work and it certainly is no cake walk, but all of this work was done by one of the attendees and as a group they certainly have the expertise to do this work. And in reality, it did not take too long. As they practice and get some of the preliminary work finished, this will be much easier.

• Spent time on one project practicing TDD and refactoring to patterns (they implemented 5 of the GoF patterns).
• Spent time practicing some of Fowler’s refactorings and Feather’s legacy refactorings.
• Spent a day practicing TDD using mocks for everything but the unit under test. At the end they had a test class, one production class and several interfaces.

In retrospect, the work they did in the first three days was nearly exactly what they needed to practice for the final day of effort. When we started tackling their legacy code, they had already practiced everything they used in getting the method under test.

So overall great week with a fun group of guys in Canada.

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!

I managed to get two walls done and everything moved back into place before I left for another business trip. My wife is happy because the bedroom looks better. I did no damage and made noticeable progress. I still have some Painting to do (the capital P is to indicate it will be a Pain). I eventually have to move the bed, rent scaffolding, and so on. That’s probably more in the future than I’d prefer, but I’ll do it when I know I have the time and space to do it.

Contrast this to when we bough the house back in March. I entered an empty house. I managed to get two bedrooms painted (ceilings included) and the “grand” room with 14’ vaulted ceilings. I only nearly killed myself once – don’t lean a ladder against a wall but put the legs on plastic – and it was much easier to move around. I had a clean slate.

Sometimes you’ve got to get done what you can get done to make some progress. When I was younger, my desire to finish the entire bedroom might have stopped me from making any progress. Sure, the bedroom is now half old paint and half new paint, but according to my wife it looks better – and she’s the product owner! I can probably do one more wall without having to do major lifting and when I’m finally ready to rent the scaffolding, I won’t have as much to do. I can break down the bed, rent the scaffolding and then in one day I might be able to finish the remainder of the work. (Well probably 2 days because I’ll end up wanting to apply 2 coats to the ceiling and I’ll need to wait 8 hours).

Painting is just like software development.

• Checks out the source tree
• Runs the script
• Starts working

So far, everything is great and this part of their build system is essential to their environment – and good in general.

Here’s the next part…

• Create the file
• Update project information
• Rerun the script to regenerate project information.

When I asked how long it takes to add a class, I was told about 5 minutes. So if I want to add a header file and source file, it takes 5 minutes. That’s a big problem. Why?

After this discussion, I heard one of the senior people lamenting that a developer had put another class in an existing file rather than creating a second file. He said something like “If the developer just had discipline, he’d to the right thing.” Those darn developers.

It takes 5 minutes to add a few files to a build. That does not include build time. That’s just the time to configure the build information.

Does 5 minutes seem like very much time?

• Some header files defined multiple classes
• Some source files had the methods for those multiple classes
• Some of the header files had names that did not match any of the classes defined in that header file

So is this a problem?

Nothing + Nothing + Nothing eventually equals something

5 minutes may not seem like a lot of time, and if it were isolated, then it’s probably not a problem. On the other hand, when you multiply it by a team and time, you end up with big problems.

Imagine, you need to use class X. Its header file is actually named Q.h and by the way, classes T U and L are defined in that file – none of which you want to know about.

So your class now has phantom dependencies on T, U and L. Also, how did you find the right header file? A quick search (time wasted). Someone changed U, so you end up having to recompile even though you don’t care about nor use (wasted time). I’m sure you can come up with a few things on your own.

So what do you do about it?

Every solution introduces problems

• Time wasting adding files
• It requires discipline to add new files, so it doesn’t always happen
• A name is wrong, but it’s a pain to update the build configuration, so it doesn’t happen – not all the time, just every so often

Little by little, things get a bit more chaotic.

So now that we’ve observed a problem – some waste, we need to find a way to remove the need to update the build information and regenerate to even work.

• Introduce interfaces
• Inversion of Control
• Identify seams and use some of Feather’s stuff to introduce them
• Etc., the usual stuff to introduce seams and break dependencies.

But many of the refactorings they’ll want to use will involve creating new classes. Since that takes a little bit longer, it will slow everything down – or seem so daunting, it might not happen at all.

Here’s a personal example. A few years ago, I built the security system for one applications and then a suite of applications with single sign on. When I initially introduced the security system, many people wrote tests that would forget to log out, causing problems in both the real system and the simulator.

I kept grumbling. I though, “if people would just do it right, there wouldn’t be a problem.” If they just had a little discipline.

• Create an abstract test base
• Change tests to use it
• In the setup, the base logged in
• In the teardown, the base logged out

• Reduced code duplication
• Increased the stability of the tests
• Made it hard for people to mess up (so long as they used the correct test base – and I updated all of the tests that needed it, so going forward, people had correct examples upon which to base their work).

Ultimately, this removed a lot of my wasted time

Detecting waste is the first thing. Until you know it is a problem, you cannot do anything about it.

• If that person was only following “the rules”
• If he/she just had a little more discipline
• Stupid user, they should not have done that

Ask yourself if it’s possible that those statements are directed at the symptom, not the problem.

I’m not smart enough to just read it from top to bottom and understand it. Sadly, when I read through and understand what in the heck the thxIniFvt variable is doing, I will forget it by the time I figure out the purpose(es) of pszVbt. I can spend all day, or even a few days to figure out a method, and that’s an admission of feeble-mindedness to be sure. I guess I’m not up to the level of some of the rapid hackers. That’s a limitation I face most days.

I find that I can sometimes understand a method like that only if I just delete all the blank lines and comments first, then reformat to break lines, then inline all methods with seven or more parameters, and then start renaming variables, extracting explanatory variables, and extracting explanatory methods. I may have to break the method into classes even. I guess I’m not one of the smart kids.

I used to be one of the smart kids. I once built a module so complex and fragile that nobody but me could figure out what to do with it. It was all tables and indicators, and stunningly clever. I am so ashamed that I wrote it. It was such a mistake that they eventually disabled it rather than field it in such a state. That was years ago, but so memorable to me. Other programmers said it was like the inside of a swiss watch, all delicate and perfectly balanced, and scary to mess with unless you first knew exactly what each part was doing, and why.

I would like to be faster than I am both mentally and in the sense of quickly producing code. I’d like to be a little less intimidated at the start of a project. .But I would not want those things if it meant building crap that people who are not appreciably more talented than myself would trip over every day. Instead, I sometimes wish I could teach the really fast, smart kids how to dumb down the code for the rest of us morons to read.

The funny thing is that dumbing code to my level doesn’t make it harder for the smart kids to use it, and sometimes allows a compiler to do a better job with it. I guess stupid isn’t so stupid after all.