Adventures in JavaScript: Getting Started

Node.jsOne of the high potentials for a Frictionless Development Environment (FDE) is Cloud9.

It is one of a growing number of web applications that uses JavaScript as the programming language for both front-end and back-end. The latter brought to you by Node.js.

So I thought it was time to start playing around with JavaScript and Node. Here is an account of my very first adventure into this Brave New World.

Preparations: Adding JavaScript Support to Eclipse

To keep the number of changes low, I wanted to keep my trusted old Eclipse. So the first step was to install Nodeclipse and jshint-eclipse.

To support documentation in the Markdown format that Node uses, I installed the Markdown Editor plugin for Eclipse.

This left me with nothing for unit tests. So I installed the JavaScript tools from Eclipse. That gave me some JS support, but nothing for creating unit tests.

Some googling told me there is such a thing as JsUnit, the JS port of my beloved JUnit. Unfortunately it doesn’t seem to come with Eclipse support, even though this thread indicates it does (or did).

JsTestDriverMaybe I’m just doing it wrong. I’d appreciate any hints in the comments.

Some more googling informed me that Orion is using JsTestDriver.

This introduction to JsTestDriver explains in detail how it works.

First Exercise: Roman Numerals

Now that I’m all set up, it’s time to do a little exercise to get my feet wet. For this I picked the Roman Numerals kata.

I started out by following this JsTestDriver example. I created a new JavaScript project in Eclipse, added src/main/js and src/test/js folders, and created the JsTestDriver configuration file:

server: http://localhost:9876

load:
  - src/main/js/*.js
  - src/test/js/*.js

Next, I opened the JsTestDriver window using Window|Show View|Other|JavaScript|JsTestDriver and started the JsTestDriver server. I then opened the client in FireFox at http://127.0.0.1:42442/capture.

The next step was to create a new run configuration: Run|Run Configurations|JsTestDriver Test. I selected the project and the JsTestDriver configuration within the project, and checked Run on Every Save.

Now everything is set up to start the TDD cycle. First a test:

RomanNumeralsTest = TestCase("RomanNumeralsTest");

RomanNumeralsTest.prototype.testArabicToRoman
    = function() {
  var romanNumerals = new TestApp.RomanNumerals();

  assertEquals("i", romanNumerals.arabicToRoman(1));
};

And then the implementation:

TestApp = { };

TestApp.RomanNumerals = function() { };

TestApp.RomanNumerals.prototype.arabicToRoman
    = function (arabic) {
  return null;
};

I completed the rest of the kata as usual.

Reflections

The cool thing about JsTestDriver is that it automatically runs all the tests every time you change something. This shortens the feedback cycle and keeps you in the flow. For Java, InfiniTest does the same.

The problem with my current tool chain is that support for renaming is extremely limited. I got Operation unavailable on the current selection. Select a JavaScript project, source folder, resource, or a JavaScript file, or a non-readonly type, var, function, parameter, local variable, or type variable.

Other refactorings do exist, like Extract Local Variable and Extract Method, but they mess up the formatting. They also give errors, but then work when trying again.

All in all I feel satisfied with the first steps I’ve taken on this journey. I’m a little worried about the stability of the tools. I also realize I have a more to learn about JavaScript prototypes.

Advertisement

How Friction Slows Us Down

FrictionI once joined a project where running the “unit” tests took three and a half hours.

As you may have guessed, the developers didn’t run the tests before they checked in code, resulting in a frequently red build.

Running the tests just gave too much friction for the developers.

I define friction as anything that resist the developer while she is producing software.

Since then, I’ve spotted friction in numerous places while developing software.

Friction in Software Development

Since friction impacts productivity negatively, it’s important that we understand it. Here are some of my observations:

  • Friction can come from different sources.
    It can result from your tool set, like when you have to wait for Perforce to check out a file over the network before you can edit it.
    Friction can also result from your development process, for example when you have to wait for the QA department to test your code before it can be released.
  • Friction can operate on different time scales.
    Some friction slows you down a lot, while others are much more benign. For instance, waiting for the next set of requirements might keep you from writing valuable software for weeks.
    On the other hand, waiting for someone to review your code changes may take only a couple of minutes.
  • Friction can be more than simple delays.
    It also rears its ugly head when things are more difficult then they ought to be.
    In the vi editor, for example, you must switch between command and insert modes. Seasoned vi users are just as fast as with editors that don’t have that separation. Yet they do have to keep track of which mode they are in, which gives them a higher cognitive load.

Lubricating Software Development

LubricationThere has been a trend to decrease friction in software development.

Tools like Integrated Development Environments have eliminated many sources of friction.

For instance, Eclipse will automatically compile your code when you save it.

Automated refactorings decrease both the time and the cognitive load required to make certain code changes.

On the process side, things like Agile development methodologies and the DevOps movement have eliminated or reduced friction. For instance, continuous deployment automates the release of software into production.

These lubricants have given us a fighting chance in a world of increasing complexity.

Frictionless Software Development

It’s fun to think about how far we could take these improvements, and what the ultimate Frictionless Development Environment (FDE) might look like.

My guess is that it would call for the combination of some of the same trends we already see in consumer and enterprise software products. Cloud computing will play a big role, as will simplification of the user interaction, and access from anywhere.

What do you think?

What frictions have you encountered? Do you think frictions are the same as waste in Lean?

What have you done to lubricate the frictions away? What would your perfect FDE look like?

Please let me know your thoughts in the comments.

Towards a Theory of Test-Driven Development

Red, Green, RefactorThis post examines how well we really understand the practice of Test-Driven Development (TDD).

Red, Green, Refactor

By now we all know that Test-Driven Development (TDD) follows a simple cycle consisting of these steps:

  1. Start by writing a test. Since there is no code, it will fail (Red)
  2. Write just enough code to make the test pass (Green)
  3. Clean up the code (Refactor)

The beauty of this division is that we can focus on one thing at a time.

Specify, Transform, Refactor

Although simple, TDD isn’t easy. To execute the TDD cycle well, we need a deeper understanding that we can only get from experience.

For instance, after doing TDD for a while we may look at the steps as:

  1. Specify new required functionality
  2. Improve the functionality while keeping the design constant
  3. Improve the design while keeping the functionality constant

When we look at the TDD cycle in this light, we see that the Green and Refactor phases are each others opposite.

Refactorings and Transformations

In the Refactor phase, we use Martin Fowler‘s refactorings to clean up the code.

TransformationRefactorings are standard alterations of the code that change its internal structure without changing its external behavior.

Now, if the Green and Refactor phases are each others opposite, then you might think that there are “opposite refactorings” as well. You would be right.

Robert Martin‘s transformations are standard alterations of the code that change its external behavior without changing its internal structure.

Automated Transformations?

Most of us use powerful IDEs to write our code. These IDEs support refactorings, which means that they can do the code alteration for you in a manner that is guaranteed to be safe.

So do we need something similar for transformations? I think not.

Some transformations are so simple in terms of the changes to code, that it wouldn’t actually save any effort to automate them. I don’t see a lot of room for improving the change from if to while, for instance.

Other transformations simply have an unspecified effect. For example, how would you automate the statement->statements transformation?

RefactoringThe crux is that refactorings keep the external behavior the same, and the tools depend on that to properly implement the refactorings. However, transformations don’t share that property.

Standardized Work

In the Specify/Transform/Refactor view of TDD, we write our programs by alternating between adding tests, applying transformations, and applying refactorings.

In other words, if we look at the evolution of our non-test code through a series of diffs, then each diff shows either a transformation or a refactoring.

It seems we are getting closer to the Lean principle of Standardized Work.

What’s still missing, however, is a deeper insight into the Red/Specify phase.

How to Write Tests

The essential part of the Red/Specify phase is obviously to write a test. But how do we do that?

For starters, how do we select the next test to implement?

Unit test failureThere is almost always more than one test to write for a given requirement.

And the order in which you introduce tests makes a difference for the implementation.

But there is very little advice on how to pick the next test, and this is sorely needed.

Kent Beck has a kata for experimenting with test order, which helps in gaining understanding. But that’s a far cry from a well-developed theory like we have for refactorings.

So what do you think? If we understood this phase better, could we come up with the test writing equivalent of transformations and refactorings?

Please share your thoughts in the comments.

The Differences Between Test-First Programming and Test-Driven Development

Red, Green, RefactorThere seems to be some confusion between Test-First Programming and Test-Driven Development (TDD).

This post explains that merely writing the tests before the code doesn’t necessarily make it TDD.

Similarities Between Test-First Programming and Test-Driven Development

It’s not hard to see why people would confuse the two, since they have many things in common.

My classification of tests distinguishes six dimensions: who, what, when, where, why, and how.

Test-First programming and Test-Driven Development score the same in five of those six dimensions: they are both automated (how) functional (what) programmer (who) tests at the unit level (where) written before the code (when).

The only difference is in why they are written.

Differences Between Test-First Programming and Test-Driven Development

Test-First Programming mandates that tests be written before the code, so that the code will always be testable. This is more efficient than having to change already written code to make it testable.

Test-First Programming doesn’t say anything about other activities in the development cycle, like requirements analysis and design.

This is a big difference with Test-Driven Development (TDD), since in TDD, the tests drive the design. Let’s take a detailed look at the TDD process of Red/Green/Refactor, to find out exactly how that differs from Test-First Programming.

Red

Unit test failureIn the first TDD phase we write a test. Since there is no code yet to make the test pass, this test will fail.

Unit testing frameworks like JUnit will show the result in red to indicate failure.

In both Test-First Programming and Test-Driven Development, we use this phase to record a requirement as a test.

TDD, however, goes a step further: we also explicitly design the client API. Test-First Programming is silent on how and when we should do that.

Green

In the next phase, we write code to make the test pass. Unit testing frameworks show passing tests in green.

In Test-Driven Development, we always write the simplest possible code that makes the test pass. This allows us to keep our options open and evolve the design.

JUnit passing testWe may evolve our code using simple transformations to increase the complexity of the code enough to satisfy the requirements that are expressed in the tests.

Test-First Programming is silent on what sort of code you write in this phase and how you do it, as long as the test will pass.

Refactor

In the final TDD phase, the code is refactored to improve the design of the implementation.

This phase is completely absent in Test-First Programming.

Summary of Differences

So we’ve uncovered two differences that distinguish Test-First Programming from Test-Driven Development:

  1. Test-Driven Development uses the Red phase to design the client API. Test-First Programming is silent on when and how you arrive at a good client API.
  2. Test-Driven Development splits the coding phase into two compared to Test-First Programming. In the first sub-phase (Green), the focus is on meeting the requirements. In the second sub-phase (Refactor), the focus is on creating a good design.

I think there is a lot of value in the second point. Many developers focus too much on getting the requirements implemented and forget to clean up their code. The result is an accumulation of technical debt that will slow development down over time.

TDD also splits the design activity into two. First we design the external face of the code, i.e. the API. Then we design the internal organization of the code.

This is a useful distinction as well, because the heuristics you would use to tell a good API from a bad one are different from those for good internal design.

Try Before You Buy

KataAll in all I think Test-Driven Development provides sufficient value over Test-First Programming to give it a try.

All new things are hard, however, so be sure to practice TDD before you start applying it in the wild.

There are numerous katas that can help you with that, like the Roman Numerals Kata.

Top-Down Test-Driven Development

In Test-Driven Development (TDD), I have a tendency to dive right in at the level of some class that I am sure I’m gonna need for this cool new feature that I’m working on. This has bitten me a few times in the past, where I would start bottom-up and work my way up, only to discover that the design should be a little different and the class I started out with is either not needed, or not needed in the way I envisioned. So today I wanted to try a top-down approach.

I’m running this experiment on a fresh new project that targets developers. I’m going to start with a feature that removes some of the mundane tasks of development. Specifically, when I practice TDD in Java, I start out with writing a test class. In that class, I create an instance of the Class Under Test (CUT). Since the CUT doesn’t exist at this point in time, my code doesn’t compile. So I need to create the CUT to make it compile. In Java, that consists of a couple of actions that are pretty uninteresting, but that need to be done anyway. This takes away my focus from the test, so it would be kinda cool if it somehow could be automated.

In work mostly in Eclipse, and Eclipse has the notion of Quick Fixes. So that seems like a perfect fit. However, I don’t want my project code to be completely dependent on Eclipse, if only because independent code is easier to test.

So I start out with a top-down test that shows how all of this is accomplished:

public class FixesFactoryTest {

  @Test
  public void missingClassUnderTest() {
    FixesFactory fixesFactory = new FixesFactory();
    Issues issues = new Issues().add(Issue
        .newProblem(new MissingType(new FullyQualifiedName("Bar")))
        .at(new FileLocation(
            new Path("src/test/java/com/acme/foo/BarTest.java"),
            new FilePosition(new LineNumber(11), new ColumnNumber(5)))));
    Fixes fixes = fixesFactory.newInstance(issues);

    Assert.assertNotNull("Missing fixes", fixes);
    Assert.assertEquals("# Fixes", 1, fixes.size());

    Fix fix = fixes.iterator().next();
    Assert.assertNotNull("Missing fix", fix);
    Assert.assertEquals("Fix", CreateClassFix.class, fix.getClass());

    CreateClassFix createClassFix = (CreateClassFix) fix;
    Assert.assertEquals("Name of new class", new FullyQualifiedName("com.acme.foo.Bar"),
        createClassFix.nameOfClass());
    Assert.assertEquals("Path of new class", new Path("src/main/java/com/acme/foo/Bar.java"),
        createClassFix.pathOfClass());
  }

}

This test captures my intented design: a FixesFactory gives Fixes for Issues, where an Issue is a Problem at a given Location. This will usually be a FileLocation, but I envision there could be problems between files as well, like a test class whose name doesn’t match the name of its CUT. For this particular issue, I expect one fix: to create the missing CUT at the right place.

I’m trying to follow the rules of Object Calistenics here, hence the classes like LineNumber where one may have expected a simple int. Partly because of that, I need a whole bunch of classes and methods before I can get this test to even compile. This feels awkward, because it’s too big a step for my taste. I want my green bar!

Obviously, I can’t make this pass with a few lines of code. So I add a @Ignore to this test, and shift focus to one of the smaller classes. Let’s see, LineNumber is a good candidate. I have no clue as to how I’ll be using this class, though. All I know at this point, is that it should be a value object:

public class LineNumberTest {

  @Test
  public void valueObject() {
    LineNumber lineNumber1a = new LineNumber(313);
    LineNumber lineNumber1b = new LineNumber(313);
    LineNumber lineNumber2 = new LineNumber(42);

    Assert.assertTrue("1a == 1b", lineNumber1a.equals(lineNumber1b));
    Assert.assertFalse("1a == 2", lineNumber1a.equals(lineNumber2));

    Assert.assertTrue("# 1a == 1b", lineNumber1a.hashCode() == lineNumber1b.hashCode());
    Assert.assertFalse("# 1a == 2", lineNumber1a.hashCode() == lineNumber2.hashCode());

    Assert.assertEquals("1a", "313", lineNumber1a.toString());
    Assert.assertEquals("1b", "313", lineNumber1b.toString());
    Assert.assertEquals("2", "42", lineNumber2.toString());
  }

}

This is very easy to implement in Eclipse: just select the Quick Fix to Assign Parameter To Field on the constructor’s single parameter and then select Generate hashCode() and equals()…:

public class LineNumber {

  private final int lineNumber;

  public LineNumber(int lineNumber) {
    this.lineNumber = lineNumber;
  }

  @Override
  public int hashCode() {
    final int prime = 31;
    int result = 1;
    result = prime * result + lineNumber;
    return result;
  }

  @Override
  public boolean equals(Object obj) {
    if (this == obj) {
      return true;
    }
    if (obj == null) {
      return false;
    }
    if (getClass() != obj.getClass()) {
      return false;
    }
    LineNumber other = (LineNumber) obj;
    if (lineNumber != other.lineNumber) {
      return false;
    }
    return true;
  }

}

This is not the world’s most elegant code, so we’ll refactor this once we’re green. But first we need to add the trivial toString():

  @Override
  public String toString() {
    return Integer.toString(lineNumber);
  }

And we’re green.

EclEmma tells me that some code in LineNumber.equals() is not covered. I can easily fix that by removing the if statements. But the remainder should clearly be refactored, and so should hashCode():

  @Override
  public int hashCode() {
    return 31 + lineNumber;
  }

  @Override
  public boolean equals(Object object) {
    LineNumber other = (LineNumber) object;
    return lineNumber == other.lineNumber;
  }

The other classes are pretty straightforward as well. The only issue I ran into was a bug in EclEmma when I changed an empty class to an interface. But I can work around that by restarting Eclipse.

If you are interested to see where this project is going, feel free to take a look at SourceForge. Maybe you’d even like to join in!

Retrospective

So what does this exercise teach me? I noted earlier that it felt awkward to be writing a big test that I can’t get to green. But I now realize that I felt that way because I’ve trained myself to be thinking about getting to green quickly. After all, that was always the purpose of writing a test.

But it wasn’t this time. This time it was really about writing down the design. That part I usually did in my head, or on a piece of paper or whiteboard before I would write my first test. By writing the design down as a test, I’m making it more concrete than UML could ever hope to be. So that’s definitely a win from my perspective.

The other thing I noted was not so good: I set out to write a top-down test, yet I didn’t. I didn’t start at the bottom either, but somewhere in the middle. I was quick to dismiss the Eclipse part, because I wanted at least part of the code to be independent from Eclipse. Instead, I should have coded all of that up in a test. That would have forced me to consider whether I can actually make the design work in an Eclipse plug-in. So I guess I have to practice a bit more at this top-down TDD stuff…

Bowling Again

It’s been a while since I’ve done the bowling game scoring exercise. For those of you who don’t know it, the bowling game (ten-pin) is almost the Hello, world! of TDD. I want to learn Groovy, but I’m going to re-do it in Java first, so that I have recent code and feelings to compare when I sink my teeth in Groovy.

Here’s the goal for this exercise: write a program that can score a bowling game. Inputs are the rolls, output the score.

The first thing to note is that the inputs are rolls (number of pins knocked down), while the score depends on frames. A frame consists of up to two rolls: if the first roll knocks all 10 pins down, then we’re talking about a strike and the frame consist of just the one roll. Otherwise, the frame consists of two rolls. If the second roll knocks down all pins, the frame is called a spare, else it’s a regular frame. The only exception is the last frame. If that is a strike or a spare, it will consist of three rolls. We call those bonus frames.

Frames

So our first task is to distinguish the several types of frames. Let’s start simple, with a regular frame:

public class RegularFrameTest {

  @Test
  public void frame() {
    final Frame frame = new RegularFrame(2, 5);
    Assert.assertEquals("# rolls", 2, frame.numRolls());
    Assert.assertEquals("First", 2, frame.pins(0));
    Assert.assertEquals("Second", 5, frame.pins(1));
  }

}

This forces me to write the Frame interface:

public interface Frame {

  int numRolls();
  int pins(int index);

}

The implementation is straightforward:

public class RegularFrame implements Frame {

  private final int first;
  private final int second;

  public RegularFrame(final int first, final int second) {
    this.first = first;
    this.second = second;
  }

  @Override
  public int numRolls() {
    return 2;
  }

  @Override
  public int pins(final int index) {
    return index == 0 ? first : second;
  }

}

Now, let’s do a spare:

public class SpareTest {

  @Test
  public void frame() {
    final Frame frame = new Spare(3);
    Assert.assertEquals("# rolls", 2, frame.numRolls());
    Assert.assertEquals("First", 3, frame.pins(0));
    Assert.assertEquals("Second", 7, frame.pins(1));
  }

}

Again, pretty easy:

public class Spare implements Frame {

  private final int first;

  public Spare(final int first) {
    this.first = first;
  }

  @Override
  public int numRolls() {
    return 2;
  }

  @Override
  public int pins(final int index) {
    return index == 0 ? first : 10 - first;
  }

}

OK, so by now, strike should be familiar territory:

public class StrikeTest {

  @Test
  public void frame() {
    final Frame frame = new Strike();
    Assert.assertEquals("# rolls", 1, frame.numRolls());
    Assert.assertEquals("first", 10, frame.pins(0));
  }

}
public class Strike implements Frame {

  @Override
  public int numRolls() {
    return 1;
  }

  @Override
  public int pins(final int index) {
    return 10;
  }

}

And finally, the bonus frame:

public class BonusFrameTest {

  @Test
  public void frame() {
    final Frame frame = new BonusFrame(10, 3, 5);
    Assert.assertEquals("# rolls", 3, frame.numRolls());
    Assert.assertEquals("first", 10, frame.pins(0));
    Assert.assertEquals("second", 3, frame.pins(1));
    Assert.assertEquals("third", 5, frame.pins(2));
  }

}
public class BonusFrame implements Frame {

  private final int first;
  private final int second;
  private final int third;

  public BonusFrame(final int first, final int second, final int third) {
    this.first = first;
    this.second = second;
    this.third = third;
  }

  @Override
  public int numRolls() {
    return 3;
  }

  @Override
  public int pins(final int index) {
    switch (index) {
      case 0: return first;
      case 1: return second;
      default: return third;
    }
  }

}

Note that I never bothered to specify what pins should return for an invalid index.

Now, there is a lot of duplication in these classes, so let’s extract a common base class. First, I’m going to focus on BonusFrame, since that one has the most code.

I’m going to take baby steps, keeping the code green as much as possible. First, I introduce a list to hold the three separate values:

public class BonusFrame implements Frame {

  private final List<Integer> pins = new ArrayList<Integer>();

  public BonusFrame(final int first, final int second, final int third) {
    this.first = first;
    this.second = second;
    this.third = third;
    pins.add(first);
    pins.add(second);
    pins.add(third);
  }

  // ...
}

Next, I implement numRolls using the new list:

public class BonusFrame implements Frame {

  @Override
  public int numRolls() {
    return pins.size();
  }

  // ...
}

Then the pins method:

public class BonusFrame implements Frame {

  @Override
  public int pins(final int index) {
    return pins.get(index);
  }

  // ...
}

Now the original fields are unused, and I can delete them safely. Never in this refactoring did I break the tests, not even for a second.

The second stage of this refactoring is to extract a base class that will hold the list of pins. Since the constructor is using the fields, I need to change it slightly, so that I can safely move the list. First I’ll introduce a new constructor that accepts a list of pins:

public class BonusFrame implements Frame {

  protected BonusFrame(final List<Integer> pins) {
    this.pins.addAll(pins);
  }

  // ..
}

Then I change the original constructor to call the new one:

public class BonusFrame implements Frame {

  public BonusFrame(final int first, final int second, final int third) {
    this(Arrays.asList(first, second, third));
  }

  // ...
}

Now I can do an automated Extract Superclass refactoring to get my coveted base class:

public class BonusFrame extends BaseFrame implements Frame {

  public BonusFrame(final int first, final int second, final int third) {
    this(Arrays.asList(first, second, third));
  }

  protected BonusFrame(final List<Integer> pins) {
    this.pins.addAll(pins);
  }

}

Unfortunately, there is no way to have Eclipse also move the constructor that I had prepared ๐Ÿ˜ฆ Also, the BaseFrame class doesn’t implement Frame yet:

public class BaseFrame {

  protected final List<Integer> pins = new ArrayList<Integer>();

  public BaseFrame() {
    super();
  }

  @Override
  public int numRolls() {
    return pins.size();
  }

  @Override
  public int pins(final int index) {
    return pins.get(index);
  }

}

This is bad, since now the code doesn’t even compile anymore, thanks to the @Override! Let’s add the missing implements, so we’re back to green. The public no-arg constructor can also go, it will be generated for us.

Now to clean up the mess. Eclipse doesn’t offer to Pull Up constructors, which is a shame. So let’s move it by hand:

public class BonusFrame extends BaseFrame implements Frame {

  public BonusFrame(final int first, final int second, final int third) {
    super(Arrays.asList(first, second, third));
  }

}
public class BaseFrame implements Frame {

  protected BaseFrame(final List<Integer> pins) {
    this.pins.addAll(pins);
  }

  // ...
}

The list should be private, and the implements Frame on BonusFrame can go.

We can now use BaseFrame as a base class for the other classes as wel:

public class Strike extends BaseFrame {

  public Strike() {
    super(Arrays.asList(10));
  }

}
public class Spare extends BaseFrame {

  public Spare(final int first) {
    super(Arrays.asList(first, 10 - first));
  }

}
public class RegularFrame extends BaseFrame {

  public RegularFrame(final int first, final int second) {
    super(Arrays.asList(first, second));
  }

}

I’m happy with the result, so let’s move on.

From Rolls To Frames

Now that we have the different kinds of frames in place, we need a way to create them from the rolls that are our input. We basically need to convert a series of rolls into a series of frames. This series idea we can capture in an Iterator:

public class FrameIteratorTest {

  @Test
  public void regular() {
    final Iterator<Frame> frames = new FrameIterator(Arrays.asList(1, 2));
    Assert.assertTrue("Missing frame", frames.hasNext());

    final Frame frame = frames.next();
    Assert.assertEquals("Frame", new RegularFrame(1, 2), frame);
    Assert.assertFalse("Extra frame", frames.hasNext());
  }

}

But for this to work, we need to implement equals on BaseFrame:

public class BaseFrameTest {

  @Test
  public void equality() {
    final Frame frame1 = new BaseFrame(Arrays.asList(4, 2));
    Assert.assertTrue("Same", frame1.equals(new BaseFrame(Arrays.asList(4, 2))));
    Assert.assertFalse("Different", frame1.equals(new BaseFrame(Arrays.asList(2, 4))));
  }

}

This is easy to implement, since Eclipse can generate the equals and hashCode for us:

public class BaseFrame implements Frame {

  @Override
  public int hashCode() {
    final int prime = 31;
    int result = 1;
    result = prime * result + ((pins == null) ? 0 : pins.hashCode());
    return result;
  }

  @Override
  public boolean equals(final Object obj) {
    if (this == obj) {
      return true;
    }
    if (obj == null) {
      return false;
    }
    if (getClass() != obj.getClass()) {
      return false;
    }
    final BaseFrame other = (BaseFrame) obj;
    if (pins == null) {
      if (other.pins != null) {
        return false;
      }
    } else if (!pins.equals(other.pins)) {
      return false;
    }
    return true;
  }

  // ...
}

Now we can return to our FrameIterator:

public class FrameIterator implements Iterator<Frame> {

  private final Iterator<Integer> rolls;

  public FrameIterator(final List<Integer> rolls) {
    this.rolls = rolls.iterator();
  }

  @Override
  public boolean hasNext() {
    return rolls.hasNext();
  }

  @Override
  public Frame next() {
    final int first = rolls.next();
    final int second = rolls.next();
    return new RegularFrame(first, second);
  }

  @Override
  public void remove() {
    throw new UnsupportedOperationException();
  }

}

OK, so how about a spare?

public class FrameIteratorTest {

  @Test
  public void spare() {
    final Iterator<Frame> frames = new FrameIterator(Arrays.asList(8, 2));
    Assert.assertTrue("Missing frame", frames.hasNext());

    final Frame frame = frames.next();
    Assert.assertEquals("Frame", new Spare(8), frame);
  }

  // ...
}
public class FrameIterator implements Iterator<Frame> {

  @Override
  public Frame next() {
    final int first = rolls.next();
    final int second = rolls.next();
    final Frame result;
    if (first + second == 10) {
      result = new Spare(first);
    } else {
      result = new RegularFrame(first, second);
    }
    return result;
  }

  // ...
}

OK, then a strike shouldn’t be too hard:

public class FrameIteratorTest {

  @Test
  public void strike() {
    final Iterator<Frame> frames = new FrameIterator(Arrays.asList(10));
    Assert.assertTrue("Missing frame", frames.hasNext());

    final Frame frame = frames.next();
    Assert.assertEquals("Frame", new Strike(), frame);
  }

  // ...
}

This test doesn’t just fail, but it throws a NoSuchElementException, since we only provide one roll. Not too hard to fix, though:

public class FrameIterator implements Iterator<Frame> {

  @Override
  public Frame next() {
    final Frame result;
    final int first = rolls.next();
    if (first == 10) {
      result = new Strike();
    } else {
      final int second = rolls.next();
      if (first + second == 10) {
        result = new Spare(first);
      } else {
        result = new RegularFrame(first, second);
      }
    }
    return result;
  }

  // ...
}

OK, that works, but the code is a bit messy. Not only do we have a magic constant, we now have it in two places! So let’s get rid of that:

public class FrameIterator implements Iterator<Frame> {

  private static final int PINS_PER_LANE = 10;

  @Override
  public Frame next() {
    final Frame result;
    final int first = rolls.next();
    if (isAllDown(first)) {
      result = new Strike();
    } else {
      final int second = rolls.next();
      if (isAllDown(first + second)) {
        result = new Spare(first);
      } else {
        result = new RegularFrame(first, second);
      }
    }
    return result;
  }

  private boolean isAllDown(final int pins) {
    return pins == PINS_PER_LANE;
  }

  // ...
}

I’m still not all that happy with this code, although I guess it does state the rules quite clearly now. Maybe the messyness of the code follows the messyness of the rules? I dont know, so let’s let our background processor think that over some more while I continue.

We now have all the normal frames in place, but we still lack the bonus frames. First a spare:

public class FrameIteratorTest {

  @Test
  public void bonusSpare() {
    final Iterator<Frame> frames = new FrameIterator(Arrays.asList(10, 10, 10, 10, 10, 10, 10, 10, 10, 2, 8, 3));
    for (int i = 0; i < 9; i++) {
      Assert.assertTrue("Missing frame " + i, frames.hasNext());

      final Frame frame = frames.next();
      Assert.assertEquals("Frame " + i, new Strike(), frame);
    }

    Assert.assertTrue("Missing bonus frame", frames.hasNext());

    final Frame frame = frames.next();
    Assert.assertEquals("Bonus frame", new BonusFrame(2, 8, 3), frame);
  }

  // ...
}
public class FrameIterator implements Iterator<Frame> {

  private static final int NUM_FRAMES_PER_GAME = 10;

  private int numFrames;

  @Override
  public Frame next() {
    numFrames++;
    final Frame result;
    final int first = rolls.next();
    if (isAllDown(first)) {
      result = new Strike();
    } else {
      final int second = rolls.next();
      if (isAllDown(first + second)) {
        if (isFinalFrame()) {
          result = new BonusFrame(first, second, rolls.next());
        } else {
          result = new Spare(first);
        }
      } else {
        result = new RegularFrame(first, second);
      }
    }
    return result;
  }

  private boolean isFinalFrame() {
    return numFrames == NUM_FRAMES_PER_GAME;
  }

  // ...
}

And then the strike:

public class FrameIteratorTest {

  @Test
  public void bonusStrike() {
    final Iterator<Frame> frames = new FrameIterator(Arrays.asList(10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 7, 2));
    for (int i = 0; i < 9; i++) {
      Assert.assertTrue("Missing frame " + i, frames.hasNext());

      final Frame frame = frames.next();
      Assert.assertEquals("Frame " + i, new Strike(), frame);
    }

    Assert.assertTrue("Missing bonus frame", frames.hasNext());

    final Frame frame = frames.next();
    Assert.assertEquals("Bonus frame", new BonusFrame(10, 7, 2), frame);
  }

  // ...
}
public class FrameIterator implements Iterator<Frame> {

  @Override
  public Frame next() {
    numFrames++;
    final Frame result;
    final int first = rolls.next();
    if (isAllDown(first)) {
      if (isFinalFrame()) {
        final int second = rolls.next();
        result = new BonusFrame(first, second, rolls.next());
      } else {
        result = new Strike();
      }
    } else {
      final int second = rolls.next();
      if (isAllDown(first + second)) {
        if (isFinalFrame()) {
          result = new BonusFrame(first, second, rolls.next());
        } else {
          result = new Spare(first);
        }
      } else {
        result = new RegularFrame(first, second);
      }
    }
    return result;
  }

  // ...
}

OK, now we really have to simplify the code! Let’s extract all the strike and spare stuff:

public class FrameIterator implements Iterator<Frame> {

  @Override
  public Frame next() {
    numFrames++;
    final Frame result;
    final int first = rolls.next();
    if (isAllDown(first)) {
      result = newStrike(first);
    } else {
      final int second = rolls.next();
      if (isAllDown(first + second)) {
        result = newSpare(first, second);
      } else {
        result = new RegularFrame(first, second);
      }
    }
    return result;
  }

  private Frame newStrike(final int first) {
    final Frame result;
    if (isFinalFrame()) {
      final int second = rolls.next();
      result = new BonusFrame(first, second, rolls.next());
    } else {
      result = new Strike();
    }
    return result;
  }

  private Frame newSpare(final int first, final int second) {
    final Frame result;
    if (isFinalFrame()) {
      result = new BonusFrame(first, second, rolls.next());
    } else {
      result = new Spare(first);
    }
    return result;
  }

  // ...
}

I think I could further simplify the code by refactoring to a Strategy pattern and then do a Replace Conditional With Polymorphism to get rid of all those pesky if statements. But although that would make the individual methods easier to read, we would lose the overview, and it may just make the rules harder to retrieve from the code. So, I’m inclined to leave it as it is.

On to scoring then!

Scoring

This is where the fun begins.

The basic score for a frame is just the number of pins that were knocked down:

public class BaseFrameTest {

  @Test
  public void baseScore() {
    final Frame frame = new BaseFrame(Arrays.asList(3, 6));
    Assert.assertEquals("Base score", 9, frame.getBaseScore());
  }

  // ...
}
public interface Frame {

  int getBaseScore();

  // ...
}
public class BaseFrame implements Frame {

  @Override
  public int getBaseScore() {
    int result = 0;
    for (final int pins : this.pins) {
      result += pins;
    }
    return result;
  }

  // ...
}

Writing that only now makes me realize that I named the list of rolls pins. But it’s not the pins knocked down, but the list of pins per roll that were knocked down:

public class BaseFrame implements Frame {

  private final List<Integer> rolls = new ArrayList<Integer>();

  protected BaseFrame(final List<Integer> rolls) {
    this.rolls.addAll(rolls);
  }

  @Override
  public int numRolls() {
    return rolls.size();
  }

  @Override
  public int pins(final int index) {
    return rolls.get(index);
  }

  @Override
  public int hashCode() {
    final int prime = 31;
    int result = 1;
    result = prime * result + ((rolls == null) ? 0 : rolls.hashCode());
    return result;
  }

  @Override
  public boolean equals(final Object obj) {
    if (this == obj) {
      return true;
    }
    if (obj == null) {
      return false;
    }
    if (getClass() != obj.getClass()) {
      return false;
    }
    final BaseFrame other = (BaseFrame) obj;
    if (rolls == null) {
      if (other.rolls != null) {
        return false;
      }
    } else if (!rolls.equals(other.rolls)) {
      return false;
    }
    return true;
  }

  @Override
  public int getBaseScore() {
    int result = 0;
    for (final int pins : rolls) {
      result += pins;
    }
    return result;
  }

}

Now, that was the easy part of scoring. Apart from the base score, strikes and spares also have extra scores. We don’t want to deal with exceptions, though. That will just lead to code littered with if statements. Instead, we can use polymorphism and the Null Object pattern:

public class BaseFrameTest {

  @Test
  public void extraScore() {
    final Frame frame = new BaseFrame(Arrays.asList(3, 4));
    Assert.assertTrue("Extra score", frame.getExtraScore() instanceof NoExtraScore);
  }

  // ...
}
public interface Frame {

  ExtraScore getExtraScore();

  // ...
}
public interface ExtraScore {

}
public class NoExtraScore implements ExtraScore {

}

Note that the ExtraScore interface is empty for now. We will get to that in a minute. First let’s make the test pass:

public class BaseFrame implements Frame {

  @Override
  public ExtraScore getExtraScore() {
    return new NoExtraScore();
  }

  // ...
}

For spares, the extra score is one roll:

public class SpareTest {

  @Test
  public void extraScore() {
    final Frame frame = new Spare(4);
    Assert.assertTrue("Extra score", frame.getExtraScore() instanceof OneRollExtraScore);
  }

  // ...
}
public class OneRollExtraScore implements ExtraScore {

}
public class Spare extends BaseFrame {

  @Override
  public ExtraScore getExtraScore() {
    return new OneRollExtraScore();
  }

  // ...
}

For strikes, the extra score is two rolls:

public class StrikeTest {

  @Test
  public void extraScore() {
    final Frame frame = new Strike();
    Assert.assertTrue("Extra score", frame.getExtraScore() instanceof TwoRollsExtraScore);
  }

  // ...
}
public class TwoRollsExtraScore implements ExtraScore {

}
public class Strike extends BaseFrame {

  @Override
  public ExtraScore getExtraScore() {
    return new TwoRollsExtraScore();
  }

  // ...
}

Allright, what about that ExtraScore interface? We don’t want to go back to dealing with rolls, since we have our lovely FrameIterator that gives us frames. So the extra score must deal with frames. First, it must tells us whether it needs any at all:

public class NoExtraScoreTest {

  @Test
  public void needFrame() {
    final ExtraScore extraScore = new NoExtraScore();
    Assert.assertFalse("Need more", extraScore.needFrame());
  }

}
public interface ExtraScore {

  boolean needFrame();

}
public class NoExtraScore implements ExtraScore {

  @Override
  public boolean needFrame() {
    return false;
  }

}

We have a little bit more work for the one roll extra score:

public class OneRollExtraScoreTest {

  @Test
  public void extraScore() {
    final ExtraScore extraScore = new OneRollExtraScore();
    Assert.assertTrue("Need more", extraScore.needFrame());

    final Frame frame = new RegularFrame(3, 4);
    final int score = extraScore.getScore(frame);
    Assert.assertEquals("Extra score", 3, score);
    Assert.assertFalse("Need still more", extraScore.needFrame());
  }

}
public interface ExtraScore {

  int getScore(Frame frame);

  // ...
}
public class OneRollExtraScore implements ExtraScore {

  private boolean needMore = true;

  @Override
  public boolean needFrame() {
    return needMore;
  }

  @Override
  public int getScore(final Frame frame) {
    needMore = false;
    return frame.pins(0);
  }

}

And the extra score for a strike is the most complex:

public class TwoRollsExtraScoreTest {

  @Test
  public void regular() {
    final ExtraScore extraScore = new TwoRollsExtraScore();
    Assert.assertTrue("Need more", extraScore.needFrame());

    final Frame frame = new RegularFrame(6, 3);
    final int score = extraScore.getScore(frame);
    Assert.assertEquals("Extra score", 9, score);
    Assert.assertFalse("Still need more", extraScore.needFrame());
  }

  @Test
  public void strike() {
    final ExtraScore extraScore = new TwoRollsExtraScore();
    Assert.assertTrue("Need more", extraScore.needFrame());

    Frame frame = new Strike();
    int score = extraScore.getScore(frame);
    Assert.assertEquals("Extra score", 10, score);
    Assert.assertTrue("Still need more", extraScore.needFrame());

    frame = new RegularFrame(6, 3);
    score = extraScore.getScore(frame);
    Assert.assertEquals("Extra score 2", 6, score);
    Assert.assertFalse("Still need more 2", extraScore.needFrame());
  }

}
public class TwoRollsExtraScore implements ExtraScore {

  private int numNeeded = 2;

  @Override
  public boolean needFrame() {
    return numNeeded > 0;
  }

  @Override
  public int getScore(final Frame frame) {
    int result = 0;
    final int numGotten = Math.min(numNeeded, frame.numRolls());
    for (int i = 0; i < numGotten; i++) {
      result += frame.pins(i);
    }
    numNeeded -= numGotten;
    return result;
  }

}

Now we need to glue all the previous together to calculate the game’s score:

public class FramesScorerTest {

  @Test
  public void score() {
    final FramesScorer scorer = new FramesScorer();
    Frame frame = new FakeFrame(2);
    scorer.add(frame);
    Assert.assertEquals("Score", 9, scorer.getScore());

    List<ExtraScore> extraScores = scorer.getExtraScores();
    Assert.assertNotNull("Missing extra scores", extraScores);
    Assert.assertEquals("# Extra scores", 1, extraScores.size());
    Assert.assertTrue("Extra score", extraScores.get(0) instanceof FakeExtraScore);

    frame = new FakeFrame(1);
    scorer.add(frame);
    Assert.assertEquals("Score 2", 19, scorer.getScore());
    extraScores = scorer.getExtraScores();
    Assert.assertEquals("# Extra scores 2", 2, extraScores.size());

    frame = new FakeFrame(0);
    scorer.add(frame);
    Assert.assertEquals("Score 3", 30, scorer.getScore());
    extraScores = scorer.getExtraScores();
    Assert.assertEquals("# Extra scores 3", 3, extraScores.size());

    scorer.add(frame);
    Assert.assertEquals("Score 4", 41, scorer.getScore());
    extraScores = scorer.getExtraScores();
    Assert.assertEquals("# Extra scores 4", 3, extraScores.size());
  }


  private static class FakeFrame extends BaseFrame {

    protected FakeFrame(final int pins) {
      super(Arrays.asList(pins, 9 - pins));
    }

    @Override
    public ExtraScore getExtraScore() {
      return new FakeExtraScore(pins(0));
    }

  }


  private static class FakeExtraScore implements ExtraScore {

    private final int numNeeded;

    public FakeExtraScore(final int numNeeds) {
      numNeeded = numNeeds;
    }

    @Override
    public boolean needFrame() {
      return numNeeded  > 0;
    }

    @Override
    public int getScore(final Frame frame) {
      return 1;
    }

  }

}

This test is a lot more involved than normal, and it took some time to get it right. This is because it’s really the sequence of steps that we want to test, as all the individual steps are already tested.

Let’s implement this assert by assert. First, the base score:

public class FramesScorer {

  private int score;

  public void add(final Frame frame) {
    score += frame.getBaseScore();
  }

  public int getScore() {
    return score;
  }

  public List<ExtraScore> getExtraScores() {
    return null;
  }

}

Next, we need to remember the extra score object:

public class FramesScorer {

  private final List<ExtraScore> extraScores = new ArrayList<ExtraScore>();

  public void add(final Frame frame) {
    score += frame.getBaseScore();
    extraScores.add(frame.getExtraScore());
  }

  public List<ExtraScore> getExtraScores() {
    return extraScores;
  }

  // ...
}

Then we need to add the collected extra scores:

public class FramesScorer {

  public void add(final Frame frame) {
    score += frame.getBaseScore();
    final Iterator<ExtraScore> iterator = extraScores.listIterator();
    while (iterator.hasNext()) {
      final ExtraScore extraScore = iterator.next();
      score += extraScore.getScore(frame);
    }
    extraScores.add(frame.getExtraScore());
  }

  // ...
}

And we must not forget to remove extra scores that are done:

public class FramesScorer {

  public void add(final Frame frame) {
    score += frame.getBaseScore();
    final Iterator<ExtraScore> iterator = extraScores.listIterator();
    while (iterator.hasNext()) {
      final ExtraScore extraScore = iterator.next();
      if (extraScore.needFrame()) {
        score += extraScore.getScore(frame);
      } else {
        iterator.remove();
      }
    }
    extraScores.add(frame.getExtraScore());
  }

  // ...
}

Actually, the way I implemented this puts the ExtraScore object on the list even if it is a NoExtraScore object, which seems a bit wasteful. So let’s fix that:

public class FramesScorerTest {

  @Test
  public void score() {
    final FramesScorer scorer = new FramesScorer();
    Frame frame = new FakeFrame(2);
    scorer.add(frame);
    Assert.assertEquals("Score", 9, scorer.getScore());

    List<ExtraScore> extraScores = scorer.getExtraScores();
    Assert.assertNotNull("Missing extra scores", extraScores);
    Assert.assertEquals("# Extra scores", 1, extraScores.size());
    Assert.assertTrue("Extra score", extraScores.get(0) instanceof FakeExtraScore);

    frame = new FakeFrame(1);
    scorer.add(frame);
    Assert.assertEquals("Score 2", 19, scorer.getScore());
    extraScores = scorer.getExtraScores();
    Assert.assertEquals("# Extra scores 2", 2, extraScores.size());

    frame = new FakeFrame(0);
    scorer.add(frame);
    Assert.assertEquals("Score 3", 30, scorer.getScore());
    extraScores = scorer.getExtraScores();
    Assert.assertEquals("# Extra scores 3", 2, extraScores.size());

    scorer.add(frame);
    Assert.assertEquals("Score 4", 41, scorer.getScore());
    extraScores = scorer.getExtraScores();
    Assert.assertEquals("# Extra scores 4", 2, extraScores.size());
  }

  // ...
}
public class FramesScorer {

  public void add(final Frame frame) {
    score += frame.getBaseScore();
    final Iterator<ExtraScore> iterator = extraScores.listIterator();
    while (iterator.hasNext()) {
      final ExtraScore extraScore = iterator.next();
      score += extraScore.getScore(frame);
      if (!extraScore.needFrame()) {
        iterator.remove();
      }
    }

    final ExtraScore extraScore = frame.getExtraScore();
    if (extraScore.needFrame()) {
      extraScores.add(extraScore);
    }
  }

  //  ...
}

Now we need to clean this up a bit:

public class FramesScorer {

  private int score;
  private final List<ExtraScore> extraScores = new ArrayList<ExtraScore>();

  public void add(final Frame frame) {
    score += frame.getBaseScore() + extraScore(frame);
    rememberExtraScore(frame);
  }

  private int extraScore(final Frame frame) {
    int result = 0;
    final Iterator<ExtraScore> iterator = extraScores.listIterator();
    while (iterator.hasNext()) {
      final ExtraScore extraScore = iterator.next();
      result += extraScore.getScore(frame);
      if (!extraScore.needFrame()) {
        iterator.remove();
      }
    }
    return result;
  }

  private void rememberExtraScore(final Frame frame) {
    final ExtraScore extraScore = frame.getExtraScore();
    if (extraScore.needFrame()) {
      extraScores.add(extraScore);
    }
  }

  // ...
}

All that’s left to do, is collect the rolls and provide them to the scorer. First the collecting:

public class GameTest {

  @Test
  public void rolls() {
    final Game game = new Game();
    game.roll(3);
    game.roll(1);
    game.roll(3);
    Assert.assertEquals("Rolls", Arrays.asList(3, 1, 3), game.getRolls());
  }

}
public class Game {

  private final List<Integer> rolls = new ArrayList<Integer>();

  public void roll(final int pins) {
    rolls.add(pins);
  }

  protected List<Integer> getRolls() {
    return rolls;
  }

}

And finally the glue that ties the scoring together:

public class GameTest {

  @Test
  public void score() {
    final Game game = new Game();
    game.roll(1);
    game.roll(1);
    game.roll(1);
    game.roll(1);
    game.roll(1);
    game.roll(1);
    game.roll(1);
    game.roll(1);
    game.roll(1);
    game.roll(1);
    game.roll(1);
    game.roll(1);
    game.roll(1);
    game.roll(1);
    game.roll(1);
    game.roll(1);
    game.roll(1);
    game.roll(1);
    game.roll(1);
    game.roll(1);
    Assert.assertEquals("Score", 20, game.getScore());
  }

}
public class Game {

  public int getScore() {
    final FramesScorer scorer = new FramesScorer();
    final FrameIterator frames = new FrameIterator(getRolls());
    while (frames.hasNext()) {
      scorer.add(frames.next());
    }
    return scorer.getScore();
  }

  // ...
}

I’m confident that the code works, but let’s make sure by adding some tests with well-known answers:

public class GameTest {

  @Test
  public void perfect() {
    final Game game = new Game();
    game.roll(10);
    game.roll(10);
    game.roll(10);
    game.roll(10);
    game.roll(10);
    game.roll(10);
    game.roll(10);
    game.roll(10);
    game.roll(10);
    game.roll(10);
    game.roll(10);
    game.roll(10);
    Assert.assertEquals("Score", 300, game.getScore());
  }

  @Test
  public void alternateStrikesAndSpares() {
    final Game game = new Game();
    for (int i = 0; i < 5; i++) {
      game.roll(10);
      game.roll(4);
      game.roll(6);
    }
    game.roll(10);
    Assert.assertEquals("Score", 200, game.getScore());
  }

}

All green, so I guess we’re done!

Reflection

Let’s take another look at all the code to see if there is anything that needs some more attention.

One thing I notice, is that the code for TwoRollsExtraScore is a generalization for the other extra scores, so we could extract a base class. First, I introduce a constructor to set the private field:

public class TwoRollsExtraScore implements ExtraScore {

  private int numNeeded;

  public TwoRollsExtraScore() {
    this(2);
  }

  protected TwoRollsExtraScore(final int numNeeded) {
    this.numNeeded = numNeeded;
  }

  // ...
}

Then use Extract Superclass (again with manual cleanup of the errors that Eclipse introduces):

public class TwoRollsExtraScore extends BaseExtraScore {

  public TwoRollsExtraScore() {
    super(2);
  }

}
public class BaseExtraScore implements ExtraScore {

  private int numNeeded;

  protected BaseExtraScore(final int numNeeded) {
    this.numNeeded = numNeeded;
  }

  @Override
  public boolean needFrame() {
    return numNeeded > 0;
  }

  @Override
  public int getScore(final Frame frame) {
    int result = 0;
    final int numGotten = Math.min(numNeeded, frame.numRolls());
    for (int i = 0; i < numGotten; i++) {
      result += frame.pins(i);
    }
    numNeeded -= numGotten;
    return result;
  }

}

And now we can use the base class to derive the other extra score classes from:

public class OneRollExtraScore extends BaseExtraScore {

  protected OneRollExtraScore() {
    super(1);
  }

}
public class NoExtraScore extends BaseExtraScore {

  protected NoExtraScore() {
    super(0);
  }

}

With that little code in the classes, one may wonder wether they are still pulling their weight. I like to think so, since they express the concepts in the game well. I introduced them because of that, after all. So I’m keeping them.

I also find a “bug” in FakeExtraScore: the numNeeded field is never decreased, so basically it will always keep requesting new frames. Here’s the fix:

public class FramesScorerTest {

  @Test
  public void score() {
    final FramesScorer scorer = new FramesScorer();
    Frame frame = new FakeFrame(2);
    scorer.add(frame);
    Assert.assertEquals("Score", 9, scorer.getScore());

    List<ExtraScore> extraScores = scorer.getExtraScores();
    Assert.assertNotNull("Missing extra scores", extraScores);
    Assert.assertEquals("# Extra scores", 1, extraScores.size());
    Assert.assertTrue("Extra score", extraScores.get(0) instanceof FakeExtraScore);

    frame = new FakeFrame(1);
    scorer.add(frame);
    Assert.assertEquals("Score 2", 19, scorer.getScore());
    extraScores = scorer.getExtraScores();
    Assert.assertEquals("# Extra scores 2", 2, extraScores.size());

    frame = new FakeFrame(0);
    scorer.add(frame);
    Assert.assertEquals("Score 3", 30, scorer.getScore());
    extraScores = scorer.getExtraScores();
    Assert.assertEquals("# Extra scores 3", 0, extraScores.size());
  }


  private static class FakeFrame extends BaseFrame {

    protected FakeFrame(final int pins) {
      super(Arrays.asList(pins, 9 - pins));
    }

    @Override
    public ExtraScore getExtraScore() {
      return new FakeExtraScore(pins(0));
    }

  }


  private static class FakeExtraScore implements ExtraScore {

    private int numNeeded;

    public FakeExtraScore(final int numNeeds) {
      numNeeded = numNeeds;
    }

    @Override
    public boolean needFrame() {
      return numNeeded  > 0;
    }

    @Override
    public int getScore(final Frame frame) {
      numNeeded--;
      return 1;
    }

  }

}

The rest of the code seems fine.

I must say that of all the times I’ve done this exercise, this time I ended up with the most classes/interfaces: 14 all together! But these total just 356 lines, or about 25 lines per type. Between all the syntactic cruft that Java makes me write and the generous use of blank lines that I prefer, I’d say that’s a pretty good score. I wonder how that will turn out in Groovy. Stay tuned.

Big Refactoring: Separate Domain from Presentation

In his landmark book Refactoring: Improving the Design of Existing Code, Marting Fowler not only presents a catalog of “regular” refactorings, he also mentions some “big” refactorings. These big refactorings are not described as a series of atomic steps to follow, but more as a recipe for using a longer series of regular refactorings. And since they are bigger than regular refactorings, they also take a lot longer to complete, sometimes even months.

I’m in the middle of one of these: Separate Domain from Presentation. Now, we all know that we shouldn’t put business logic in interface code, so why do I find myself in this situation?

Well, technically, I don’t ๐Ÿ˜‰ We use Struts, which has a nice MVC architecture. However, it’s the Controller part that has me worried. In Struts, one writes Action classes to control application flow:

“The goal of an Action class is to process a request, via its execute() method, and return an ActionForward object that identifies where control should be forwarded (e.g. a JSP, Tile definition, Velocity template, or another Action) to provide the appropriate response.”

It is, however, all too convenient to implement the business logic in Action classes as well. The Struts documentation even warns about this danger:

“Perform the processing required to deal with this request (such as saving a row into a database). This can be done by logic code embedded within the Action class itself, but should generally be performed by calling an appropriate method of a business logic bean.”

And that’s exactly what’s happened in our code. So I guess I’m actually in the middle of Separate Domain from Controller ๐Ÿ˜‰

Fixing this is not a trivial task. The Action classes use ActionForm classes that hold data entered in the UI to perform their work. This ties them to Struts, which I don’t like at all. For instance, it makes it very hard for us to switch to a different web framework, should we so choose. It also means that simple solutions like Extract Method won’t work, since the extracted method would get the ActionForm as a parameter.

My solution has been to introduce what I call Service classes. A Service class has one method that implements the service that the Action provides. The method has one parameter, which is a Parameter Object, that contains the same information as the Action‘s ActionForm does. I call them Service classes, since these classes could very well be used to implement web services as well.

Anyway, all the Action class has to do now, is:

  1. instantiate the appropriate Parameter Object
  2. populate it from the ActionForm
  3. instantiate the appropriate Service class
  4. call the appropriate method on the Service class (passing the Parameter Object)
  5. update the ActionForm from the method’s result object
  6. construct an ActionForward (possibly using information from the result object)

Luckily, I could automate all of that in a base class using reflection, so that each Action class now only needs two methods: one for instantiating the Parameter Object, and one for instantiating the Service class.

Still, that leaves a lot of Actions to convert. And to make matters worse, they are organized into class hierarchies, which makes it hard to convert them one by one. So I guess I won’t be sitting idle any time soon…

The Law of Demeter

In my previous post I used the Law of Demeter as a motivation for the Hide Delegate refactoring:

The Law of Demeter for functions requires that a method M of an object O may only invoke the methods of the following kinds of objects:

  1. O itself
  2. M’s parameters
  3. any objects created/instantiated within M
  4. O’s direct component objects

Code that violates the Law of Demeter is a candidate for Hide Delegate, e.g. manager = john.getDepartment().getManager() can be refactored to manager = john.getManager(), where the Employee class gets a new getManager() method.

However, not all such refactorings make as much sense. Consider, for example, someone who’s trying to kiss up to his boss: sendFlowers(john.getManager().getSpouse()). Applying Hide Delegate here would yield a getManagersSpouse() method in Employee. Yuck.

I have a couple of problems this use of Hide Delegate. First of all, it creates methods that by definition reek of feature envy. Second, methods like getManagersSpouse() clearly violate the single responsibility principle. Finally, the Law of Demeter clashes with the concept of fluent interfaces.

Luckily, you can always back out from an adverse Hide Delegate by applying the opposite refactoring: Remove Middle Man.

Automating refactorings

I’m a big fan of both refactoring and automation. It’s no wonder, then, that the support for automated refactoring in Eclipse makes me very happy. I find that it makes me a lot more productive, and I produce better code. That’s because performing a refactoring is easy and fast enough to actually do it.

I also find that I refactor routinely. Where Martin Fowler, in his classic book Refactoring, gives the advice not to mix refactoring and adding new functionality, I do it almost mindlessly anyway. No need to run unit tests before and after the refactorings, since I know they Just Work™.

Not so with any refactorings that are not supported by the tool, though. For instance, when trying to adhere to the Law of Demeter, one would want to perform the refactoring Hide Delegate. Unfortunately, Eclipse has no support for this refactoring ๐Ÿ˜ฆ You can, however, simulate this refactoring using a combination of other refactorings. Let me explain that using a simple example.

We start with the following abstract code that shows the situation before we want to apply Hide Delegate:

public class Client {

  public void example() {
    final Server server = new Server();
    server.getDelegate().method();  
  }

}

public class Server {

  private final Delegate delegate = new Delegate();

  public Delegate getDelegate() {
    return delegate;
  }
  
}

public class Delegate {

  public void method() {
   // Do it...
  }
  
}

First, we perform Extract Method on server.getDelegate().method() (make the method public):

public class Client {

  public void example() {
    final Server server = new Server();
    method(server);  
  }

  public void method(final Server server) {
    server.getDelegate().method();
  }

}

Next, perform Move Method to move method() to Server:

public class Client {

  public void example() {
    final Server server = new Server();
    server.method();  
  }

}

public class Server {

 private final Delegate delegate = new Delegate();

  public Delegate getDelegate() {
    return delegate;
  }

  public void method() {
    getDelegate().method();
  }
  
}

And, voila, we have performed Hide Delegate!