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.

Background

When I was working through this example, I was a little exception happy. I was trying to learn how to make certain things happen with the moq. I’ve simply repeated my experiments here, when I use this later on, I’ll approach this problem a bit differently.

Example 1: Logging in successfully

00: [Test]
01: public void WhenUserFoundAndPasswordMatchesUserShouldBeLoggedIn()
02: {
03:   var userMock = new Mock<IUser>();
04:   userMock.Setup(user => user.PasswordMatches(It.IsAny<string>())).Returns(true);
05:
06:   var daoMock = new Mock<IUserDao>();
07:   daoMock.Setup(dao => dao.Get(It.IsAny<string>())).Returns(userMock.Object);
08:
09:   var service = new UserService(daoMock.Object);
10:   service.Login("Brett", "password");
11:
12:   userMock.VerifySet(user => user.LoggedIn = true);
13: }

Notice that rather than setting expectations, line 12 verifies only what I want to verify. It is possible to call Setup more times and then use “VerifyAll” to confirm all the Setup’s were actually used. However, that makes the test more coupled to the underlying implementation. All I care about is that as a result of this configuration of objects, a user was LoggedIn.

By default, mocks are not strict. That is, code may call any methods it wishes on a give mock object. So the default behavior is more like a stub than a mock object. If you call “VerifyAll” on the userMock or the daoMock, then all of the descriptions in the various Setup method invocations need to happen.

Example 2: User not found? Throw an exception

00: [Test]
01: [ExpectedException(typeof(UserDoesNotExistException))]
02: public void WhenUserNotFoundShouldThrowException()
03: {
04:   var daoMock = new Mock<IUserDao>();
05:   daoMock.Setup(dao => dao.Get(It.IsAny<string>())).Returns((IUser) null);
06:
07:   var service = new UserService(daoMock.Object);
08:   service.Login("Brett", "password");
09: }

Notice that this example includes another non-strict mock. You can call it as often as you wish and there’s no checking on the methods called. However, this stub should force the implementation of the Login method to throw a UserDoesNotExist exception.

Example 3: Throw an Exception if the password does not match

00: [Test]
01: [ExpectedException(typeof(PasswordDoesNotMatchException))]
02: public void WhenLoginAttemptDoesNotMatchPasswordThrowsException()
03: {
04:   var userMock = new Mock<IUser>();
05:   userMock.Setup(user => user.PasswordMatches(It.IsAny<string>())).Returns(false);
06:
07:   var daoMock = new Mock<IUserDao>();
08:   daoMock.Setup(dao => dao.Get(It.IsAny<string>())).Returns(userMock.Object);
09:
10:   var service = new UserService(daoMock.Object);
11:   service.Login("", "");
12: }

Eample 4: Three failed login attempts should cause user account to be revoked.

00: [Test]
01: public void WhenLoginAttemptThreeTimesInARowWithInvalidPasswordUserRevoked()
02: {
03:   var userMock = new Mock<IUser>();
04:   userMock.Setup(user => user.PasswordMatches(It.IsAny<string>())).Returns(false);
05: 
06:   var daoMock = new Mock<IUserDao>();
07:   daoMock.Setup(dao => dao.Get(It.IsAny<string>())).Returns(userMock.Object);
08:
09:   var service = new UserService(daoMock.Object);
10:   for (int i = 0; i < 3; ++i)
11:     AttempLoginIgnoringException(service, "");
12:   
13:   userMock.VerifySet(user => user.Revoked = true);
14: }

Example 5: Failing 2x with one account and then switching to another account and failing again does not revoke account.

00: [Test]
01: public void LoginWrongPassword2XandThenSwitchAccountsWithWrongPassowrdNothingRevoked()
02: {
03:   var userMock1 = new Mock<IUser>();
04:   userMock1.Setup(user => user.PasswordMatches(It.IsAny<string>())).Returns(false);
05:
06:   var userMock2 = new Mock<IUser>(MockBehavior.Strict);
07:   userMock2.Setup(user => user.PasswordMatches(It.IsAny<string>())).Returns(false);
08: 
09:   var daoMock = new Mock<IUserDao>();
10:   daoMock.Setup(dao => dao.Get("user1")).Returns(userMock1.Object);
11:   daoMock.Setup(dao => dao.Get("user2")).Returns(userMock2.Object);
12:
13:   var service = new UserService(daoMock.Object);
14:
15:   AttempLoginIgnoringException(service, "user1");
16:   AttempLoginIgnoringException(service, "user1");
17:   AttempLoginIgnoringException(service, "user2");
18:
19:   userMock2.VerifyAll();
20: }

Conclusion

• A book may have a receipt (this is a poor design, that’s part of the exercise).
• A receipt may have a book.

Both sides of the relationship are 0..1. After creating a receipt, I end up with a circular reference between Receipt and Book.

In the existing Java and C# implementations, there was no cleanup code in the test teardown to handle what happens when the receipt goes away. This was not a problem since C# and Java garbage collection algorithms easily handle this situation.

Shared pointers, however, do not handle this at all. They are good, sure, but not as good as a generation-scavenging garbage collector (or whatever algorithms are used these days – I know the JVM for 1.6 sometimes uses the stack for dynamic allocation based on JIT, so it’s much more sophisticated than a simple generation-scavenger, right?)

boost::shared_ptr<Receipt> r = ...;
CHECK(xxx, yyy);
r.setCopy(boost::shared_ptr<Book>());

(I did not use these types like this. When I use templates, especially those in a namespace, I use typedefs and I even, gasp, use Hungarian-esque notation.)

That would work, though it is ugly. Also, it is error prone and will either require violating DRY or making an automatic variable a field.

I could have removed the back reference from the Receipt to the book. That’s OK, but is a redesign of a system deliberately written with problems (part of the assignment).

Maybe I could explicitly “return” the book, which could remove the receipt and the back-reference. That would make the test teardown a bit more complex (and sort of upgrade the test from a unit test to something closer to an integration test), but it makes some sense. The test validate borrowing a book, so to clean up, return the book.

Instead of any of these options, I decided to use a boost::weak_ptr on the Receipt side. (This is the “technology to the rescue solution”, thus my question, is this an OK approach.)

I did this since the lifetime of a book object is much longer than its receipt (you return your library books, right?). Also, the Receipt only exists on a book. But the book could exist indefinitely without a Receipt.

This fixed the problem right away. I got a clean run using CppUTest. All tests passed and no memory leaks.

aReceipt->getBook()->getIsbn();

(Yes, violating Law of Demeter, get over it, the alternative would make a bloated API on the Book class.)

aReceipt->getBook().lock()->getIsbn();

The lock() method promotes a weak_ptr to a shared_ptr for the life of the expression. In this case, it’s a temporary in that line of code.

boost::shared_ptr<Book> getBook() {
    return book.lock();
}

On the one hand, anybody using the getBook() method is paying the price of the promotion. However, the weak_ptr doesn’t allow access to its payload without the promotion so it’s really required to be of any value. Or at least that’s my take on it.

Do you have different opinions?

Please keep in mind, this is example code we use in class to give students practice naming things like methods and variables and also practice cleaning up code by extracting methods and such.

Even so, what practice do you use, if any, when using shared_ptr? Do you use weak_ptr?

Thanks in advance for your comments. I’ll be reading and responding as they come up.

Anyway, the point at which the information was available that I needed and the point where I needed to use the information were may steps away from each other both in the call stack as well as the instance hierarchy (think composite pattern). I did not want to significantly change the method call hierarchy so I instead decided to hand-roll the wormhole pattern as it manifests itself in AspectJ (and not the wormhole anti-pattern).

For background on AspectJ and AOP in general, have a look at these self-study tutorials.

The wormhole pattern is well-documented in AspectJ In Action. It consists of two point cuts that combine two different, separated, parts of the system. It grabs information available at (in this case) the entry point to the system and makes that information available deeper in the system without passing the parameter directly. That’s where the name comes from, the pattern bridges information across to unconnected points via the wormhole. It is my favorite patter name (not favorite pattern, just name).

AspectJ happens to store certain runtime information in thread local storage and the wormhole pattern exploits this fact. Here’s another example that’s close to the wormhole pattern. This is actually a common technique. If you’ve read up on the recommendations on using Hibernate in a JSE environment either in “Hibernate in Action” or the more recent Javer Persistence with Hibernate, one recommendation is to pass around sessions and such in thread local variables. Under the covers, JEE containers do the same thing.

Even though this is a “common” technique, I’m not a huge fan of using thread-locals. 1. They are thread-specific global variables. 1. You better be sure there’s no threading between the two points. In this case, the threading has already happened. Once a FitNesse responder gets a hold of an HTTP request, the remaining processing is in a single thread.

On the other hand, if I did not use thread local storage, the change required to get information I needed would have either require changing one of the objects already in the parameters being passed (very ugly) or changing method signatures all over the place (somewhat less ugly but a stronger violation of LSP). So in this case, I think thread local variables are the least ugly of the available options.

(As a side note, that’s my definition of design: Select the solution that sucks the least.)

If you’re like me, you don’t use thread locals very often. I wanted to make sure that the thread local information would get properly cleaned up and I wanted to hide all of the magic in one place, so I created a simple class called ThreadLocalUtil. I used TDD to create the class and when I though I was done, I wanted to make sure that I had written the clear() method correctly. I knew that for a single thread the clear() method worked as expected, but I wanted to make sure it did not affect other threads.

• T1: Store value in thread-local storage.
• T2: Clear its local storage.
• T1: Read its local storage, value stored should still be available.

package fitnesse.threadlocal;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;

import java.util.concurrent.CountDownLatch;

import org.junit.After;
import org.junit.Before;
import org.junit.Test;

public class ThreadLocalUtilTest {
  private String valueFound;

  // snip - several other tests removed to focus on this test

  CountDownLatch t1Latch = new CountDownLatch(1);
  CountDownLatch t2Latch = new CountDownLatch(1);

  class T1 implements Runnable {
    public void run() {
      try {
        ThreadLocalUtil.setValue("t1", "value");
        t2Latch.countDown();
        Thread.yield();
        t1Latch.await();
        valueFound = ThreadLocalUtil.getValue("t1");
      } catch (InterruptedException e) {
        e.printStackTrace();
      }
    }
  }

  class T2 implements Runnable {
    public void run() {
      try {
        t2Latch.await();
        ThreadLocalUtil.clear();
        t1Latch.countDown();
      } catch (InterruptedException e) {
        e.printStackTrace();
      }
    }
  }

  @Test
  public void assertThatClearInOneThreadDoesNotMessUpAnotherThread()
      throws InterruptedException {
    Thread t1 = new Thread(new T1());
    Thread t2 = new Thread(new T2());
    t1.start();
    t2.start();
    t1.join();
    t2.join();
    assertEquals("value", valueFound);
  }
}

This example illustrates using the java.util.concurrent.CountDownLatch class to signal between threads.

• Creates two threads and start them.
• Wait for both threads to complete.

• If T2 starts running before T1, no problem it waits on the countdown latch.
• When T1 starts, it stores a value in a thread local using the util class.
• T1 then counts down, releasing T2.
• T1 then yields, it is done until its countdown latch is signaled.
• T1 waits for a signal.
• T2 is released from its countdown latch.
• T2 sends clear to the thread local util class (there’s a test that verifies clear() works as named in a single thread.
• T2 then signals T1 to continue by calling its countdown latch.
• T2 completes at some point later.
• T1 starts running again, grabs the value out of thread local storage and puts it in the valueFound.
• T1 completes.

• The completion of T1 and T2 make the join methods called in the original test method to return, whereupon the test can complete.
• Verifies that the variable valueFound, set in T1.run, stores the expected result.

This kind of hand-rolled synchronization is certainly error prone. This is where having a second pair of eyes can really help. However, this seemed to verify that the clear method as written worked in multiple threads as expected.

If you’re interested in seeing the ThreadLocalUtil class or ThreadLocalUtilTest class, grab a copy of FitNesse from github.