OO design question?

[Ilja Preuss]

Originally posted by Frank Carver:
OK. I have signed up. What I'll do (if you have no objections) is work through the history of changes to the files (I have kept a source "snapshot" each time we handed over) checking them in, so that the history of the discussion is reflected in the svn history.

Oops, we have a race condition here...

[Frank Carver]

Never mind. We can sort that out later. I've now got an eclipse project hooked up to the source in svn, so I'm ready to move on.

Meanwhile I'm still lookimng forward to your next move - will it be a refactoring? a new test? a change of emphasis? all of the above?

[Ilja Preuss]

We will hopefully found out this evening!

[Ilja Preuss]

Sorry, didn't find the time yesterday - had to watch "Pirates of the Caribbean 2" with my best friend...

One minor thing I'm thinking about is restructuring the packages.

What would you think about

com.javaranch.tennis
+- functionaltests
|
+-+ model
| +- tests
|
+-+ view
  +- tests

[ August 01, 2006: Message edited by: Ilja Preuss ]

[Frank Carver]

I've seen some projects with this sort of structure, but I've never been sure of the benefit. My ow projects tend to sit with whatever seems an appropriate domain structure for the delivered code, but just a single package for tests. Well, OK. I do sometimes extract out a separate package for mock implementations, as they can often be copied/reused for other projects. But I've never really seen the advantage of mirroring the domain structure in the tests

e.g.

/src/delivery/java
[+] org.javaranch.tennis
    GameView.java
    ScoreCard.java
      ...
/src/test/java
[+] tests
    AllTests.java
    TennisFuctionalTest.java
    TennisGameTest.java
[+] mock
    MockGameView.java

Can you explain a bit about your thinking behind your suggestion? Even better, can you drive it with a test?

[Ilja Preuss]

Mhh, I don't know - it's just what I'm used to.

OK, let's try the existing structure for some while.

[Ilja Preuss]

There are some things that I don't fully like

- the playerNumber and playerIndex methods in TennisContest scare me,
- we don't have unit tests for TennisSet, and
- I don't think that the GameView should have the responsibility of incrementing the set score when a game is won.

I think I will try to tackle the last two, by writing a unit test for TennisSet that makes it responsible for updating the score.

I create an empty TennisSet class and add it to the TestSuite. Just to see that it works, I run the suite and get the expected warning that I have an empty test class.

So let's add a test:

public void testScoreAfterPlayerWonAGame() throws Exception {
TennisSet tennisSet = new TennisSet();
tennisSet.gameWin(0);
assertEquals(1, tennisSet.playerOneScore());
}

That doesn't compile yet. Eclipse creates the missing method stub for us.

Now the test fails, as expected: junit.framework.AssertionFailedError: expected:<1> but was:<0>

Implementing gameWin is easy:

public void gameWin(int player) {
point(player);
}

Ups, test still fails: java.lang.ArrayIndexOutOfBoundsException: -1

I forgot that the player number starts by one, not zero - I swear, this wasn't intentional!

Let's correct the test:

public void testScoreAfterPlayerWonAGame() throws Exception {
TennisSet tennisSet = new TennisSet();
tennisSet.gameWin(1);
assertEquals(1, tennisSet.playerOneScore());
}

OK, test runs.

Now TennisSet is able to keep track of won games - we only to use it and relieve the view from this responsibility. If I remember correctly, we just need to change the setup of the FunctionTest:

public void setUp() {
game = new TennisGame();
set = new TennisSet();
scoreCard = new ScoreCard(game, set);
game.listen(set);
}

Oops, two problems: TennisSet doesn't implement the interface yet, and the listen method is a misnomer - it's the set which listens to the game, not the other way around.

Let's fix the former first. TennisSet needs to implement GameView.

OK, compiles; tests run.

Now to the refactoring: Let's go with existing Java idioms and call the method addGameListener. As I think about it, there is only one listener allowed, currently, so it should be setGameListener.

That's better. Mhh, the interface then also shouldn't be named GameView, but GameListener.

OK. I also renamed the variables and fields from "view" to "listener". God bless the inventor of refactoring browsers.

Oh, I nearly forgot to rename the MockGameView to MockGameListener. Done.

Tests still run.

The whole point of the exercise was to remove the responsibility from the ScoreCard. Now it doesn't need to implement the View/Listener interface any longer.

I removed the code. Everything still compiles, tests run. Enough for today.

[Ilja Preuss]

Originally posted by Ilja Preuss:
- I don't think that the GameView should have the responsibility of incrementing the set score when a game is won.

I meant the ScoreCard, not the GameView, of course... :roll:

[Ilja Preuss]

I have updated the wiki page http://faq.javaranch.com/view?TennisMatchCode to link directly to the uptodate code in the repository. We still need to update the page when we add classes or change their names.

[Frank Carver]

Neat. I'm just having a got at my response to your changes ...

[Ilja Preuss]

My dictionary doesn't know what "having a got at smthg." means...

[Frank Carver]

Ilja wrote: the playerNumber and playerIndex methods in TennisContest scare me

So that's the first item on my list then.

The offending methods look like:

    protected int playerIndex(int player) {
        return player-1;
    }
 
    protected int playerNumber(int player) {
        return player+1;
    }

I agree. These are horrible. The -1 and +1 are purely there to adapt between a domain concept (player number) and an implementation concept (array index). There must be a better way.

Luckily, now that these methods have been abstracted into a common base class, we are almost in a position to change them without impacting other classes. Unfortunately, a quick check shows that "playerIndex" is used in TennisSet, and "playerNumber" is used in TennisGame. Let's start by bringing all those references home to TennisContest.

In TennisSet we have:

    public void point(int player) {
        ++ score[playerIndex(player)];
    }

An "extract method" gives:

    public void point(int player) {
        addPoint(player);
    }
 
    private void addPoint(int player) {
        ++ score[playerIndex(player)];
    }

Then it's a simple matter to "pull up" the new method to TennisContest, co-incidentally changing its visibility to "protected".

Run the tests. Good, all still pass.

In TennisGame, the situation is a little more complicated. It's not obviously amenable to an extract-and-move. The offending call is in:

    private void notifyWinner(int player) {
        if (listener != null) {
            listener.gameWin(playerNumber(player));
        }
}

The trick here, I think, is to "pull up" all the winner-notification stuff:

    private GameListener listener;
 
    public void setGameListener(GameListener listener) {
        this.listener = listener;
    }
 
    private void notifyWinner(int player) {
        if (listener != null) {
            listener.gameWin(playerNumber(player));
        }
    }

Sure the names will be a bit out of place, but we can fix that when we have made the main change.

To prove that we have achieved the goal of removing all external references to our two problem methods, change them to private:

    private int playerIndex(int player) {
        return player-1;
    }
 
    private int playerNumber(int player) {
        return player+1;
    }

and Re-run the tests again. Good.

OK, that's the first step done. Now, to solve this properly we need to change how scores are stored. An array indexed by "player" seemed a reasonable idea at the start, but now it is causing trouble, so it has to go.

However, in order to achieve that, we also need to "bring home" external references to the score array.

Most of the references to this field are in TennisContest, which is fine. Unfortunately, there are also two references in TennisGame:

    private void point(int player) {
        if (score[player] == 3) {
            reset();
            notifyWinner(player);
        } else {
            ++score[player];
        }
    }

Let's change those array references to methods for the moment, usnig the asme extract-and-move as before:

    private void point(int player) {
        if (getScore(player) == 3) {
            reset();
            notifyWinner(player);
        } else {
            addScore(player);
        }
    }
 
    private int getScore(int player) {
        return score[player];
    }
 
    private int addScore(int player) {
        return ++score[player];
    }

This is all beginning to feel a bit clumsy, but trust me, it will get simpler in the end.

Again, I check this by marking the score array as private in TennisContest and re-running the tests. Good.

Next step, change "score" from an array to a Map:

    private Map score = new HashMap();

This naturally enough gives a bunch of compilation errors. The good thing is that because we made sure all the references were moved back to TennisContest, that's the only class with errors. So let's fix them:

For example:

    public int playerOneScore() {
        return score[PLAYER_ONE];
    }

becomes:

    public int playerOneScore() {
        return ((Integer)score.get(new Integer(PLAYER_ONE))).intValue();
    }

Looks worse, doesn't it? Don't worry. Get the code compiling again first, then we can address these issues.

To get the code compiling quickly, I ended up with this:

    public int playerOneScore() {
        return ((Integer)score.get(new Integer(PLAYER_ONE))).intValue();
    }
 
    public int playerTwoScore() {
        return ((Integer)score.get(new Integer(PLAYER_TWO))).intValue();
    }
 
    protected void reset() {
        score.put(new Integer(PLAYER_ONE), new Integer(0));
        score.put(new Integer(PLAYER_TWO), new Integer(0));
    }
 
    protected void addPoint(int player) {
        score.put(new Integer(player), new Integer(((Integer)score.get(new Integer(player))).intValue()+1));
    }
 
    protected int getScore(int player) {
        return ((Integer)score.get(new Integer(player))).intValue();
    }
 
    protected void addScore(int player) {
        score.put(new Integer(player), new Integer(((Integer)score.get(new Integer(player))).intValue()+1));
    }

Compilation is good, but unfortunately all the tests now fail.

Remember when I changed the score from an array to a map? I took away the bit that set the values to zero. We could do the equivalent in an initialiser block, but I perfer a constructor:

    public TennisContest() {
        reset();
    }

That fixes most of them. But there are still a few errors. It looks as if the failing ones are the three which test that a Set is updated when a player wins a game (one in TennisSetTest, and two in TennisFunctionalTest).

A diagnostic print in TennisSet shows that Win is being called:

    public void gameWin(int player) {
System.out.println("TennisSet.gameWin(" + player +")");
        point(player);
    }

I reckon this is all still to do with the two types of player number. And guess what. "notifyWinner" still calls "playerNumber" which we thought we had got rid of:

    protected void notifyWinner(int player) {
        if (listener != null) {
            listener.gameWin(playerNumber(player));
        }
    }

So let's remove that call:

    protected void notifyWinner(int player) {
        if (listener != null) {
            listener.gameWin(player);
        }
    }

That, and changing the symbolic constants to be the "real" player numbers fixes all the tests:

    static protected final int PLAYER_ONE = 1;
    static protected final int PLAYER_TWO = 2;

Best of all we can now remove the horrible conversion methods that prompted all this in the first place.

We still need to clear up that mess of object/int conversions, though.

All our tests are working, and the code is in the source-repository, so let's take a bold step. Let's change the player symbols to Objects. For clarity in diagnostics, let's make them actually Strings, but make sure that they are always passed around as opaque objects.

This does affact a few classes in our system, but the changes are not hard.

    static protected final Object PLAYER_ONE = "Player One";
    static protected final Object PLAYER_TWO = "Player Two";

After that, most of the changes are removing the "new Integer()" wherever PLAYER_ONE or PLAYER_TWO are used, and changing the type of a few parameters. Atter that it's just a matter of replacing player numbers with text in the tests.

Eventually the tests run again. And as a nice confidence boster, that diagnostic print is still there in TennisSet, and prints out:

TennisSet.gameWin(Player One)
TennisSet.gameWin(Player One)
TennisSet.gameWin(Player Two)

Remove it anyway, so the tests run clean.

It might look like a good place to stop, after all that, but there is still a little more. Remember I complained about all those little methods "point", "addPoint", "playerOneWinsThePoint" all doing very similar things? Well I reckon we can get rid of most of them. Our new player objects are almost good enough to be used as a public interface. All that is needed is for the text of the strings to make sense, rather than being something chosen by me. So let's make them constructor parameters:

    protected Object PLAYER_ONE;
    protected Object PLAYER_TWO;
    
    private Map score = new HashMap();
    private GameListener listener;

    public TennisContest(String playerOne, String playerTwo) {
        PLAYER_ONE = playerOne;
        PLAYER_TWO = playerTwo;
        reset();
    }

These parameters need to be rippled up through TennisGame and TennisSet.

Tests all work again. Good.

I think we are nearly done, but one thing still bugs me. In TennisFunctionalTest we now have:

    public void setUp() {
        game = new TennisGame("Player One", "Player Two");
        set = new TennisSet("Player One", "Player Two");
        scoreCard = new ScoreCard(game, set);
        game.setGameListener(set);
    }

All that set up stuff looks too ugly and detailed for a functional test, and ther's a worrying chance of the player symbols being different in the Game and Set, which could cause horrible bugs. So I'll move it into the ScoreCard constructor:

    public ScoreCard(String playerOne, String playerTwo) {
        game = new TennisGame(playerOne, playerTwo);
        set = new TennisSet(playerOne, playerTwo);
        game.setGameListener(set);
    }

This leaves the functional test setUp much nicer:

    public void setUp() {
        scoreCard = new ScoreCard("Player One", "Player Two");
    }

However, the tests currently call the "..wins.." methods dierctly on a game object. Let's simplify them into a single method, then use the ScoreCard as a proxy:

in TennisGame:

    public void point(Object player) {
        if (getScore(player) == 3) {
            reset();
            notifyWinner(player);
        } else {
            addScore(player);
        }
    }

in ScoreCard:

    public void point(String player) {
        game.point(player);
    }

A few minor changes to the tests to use the new interfaces, and removal of the old ones, and the code is smaller again.

And all the tests run. Time to check in again and get back to some other work.
[ August 03, 2006: Message edited by: Frank Carver ]

[Frank Carver]

My dictionary doesn't know what "having a got at smthg." means...

It means I sometimes type before thinking. I should have been "having a go".

Sorry.

[Ilja Preuss]

Originally posted by Frank Carver:

It means I sometimes type before thinking. I should have been "having a go".

Well, you are not alone - google has an incredibly high hit count for "having a got at"...

[Ilja Preuss]

Mhh, would it be OK if we used Java 5 for this exercise?

[Frank Carver]

I guess so.

Do you want it just for the autoboxing, or for something else?

[Ilja Preuss]

Generics could simplify the use of the score Map. And I'd like to see what happens if we make the players an enum.

[Ilja Preuss]

OK, time for a little bit new functionality. Let's tackle "deuce"!

public void testDeuceInFirstGame() {
scoreCard.point("Player Two");
scoreCard.point("Player Two");
scoreCard.point("Player Two");
scoreCard.point("Player One");
scoreCard.point("Player One");
scoreCard.point("Player One");
assertEquals("0-0 0-1 Deuce", scoreCard.fullCard());
}

That should fail. And it does: junit.framework.ComparisonFailure: expected:<...1 Deuce> but was:<...0 40-40>

Oops, there is something wrong in this test - a typical copy-and-past error. Let's correct that:

assertEquals("0-0 0-0 Deuce", scoreCard.fullCard());

Now the test fails with the expected error message: junit.framework.ComparisonFailure: expected:<...Deuce> but was:<...40-40>

It will take some time to make that test succeed, but I don't want to always check the number of failed tests when working test first at the unit level. So I will just remove this test from the suite again, by changing its name:

public void _testDeuceInFirstGame() {

Now we are green again, but know about our goal. When we think we have reached it, we can add the test again and see whether we are right!

[Ilja Preuss]

OK, we first need a new test for TennisGame. But when I open the TennisGameTest, I see a lot of duplication. Refactoring tests is as important as refactoring production code, so I'd like to tackle that first.

One thing that bugs me are the player strings all over the place. In fact I think those shouldn't be strings at all - I prefer it to use the compilers ability to find my typing errors. (I know that this is unusual for someone from the more extreme corner of the Agile community - call it my personal quirk... ).

I will commit the current state of the code and then try to introduce an enum. If we don't like the result, we can always roll back the change.

The Player enum is really simple:

package org.javaranch;
 
public enum Player {
  ONE, TWO;
}

We now could look for a smooth migration path from Strings to the Player enum, but I feel jaunty (?) today, so I will try to change it in one big swoop. I'll just use the text "find and replace" functionality.

After the text replacement, I need to do an "organize imports" for the tests, and change all the constructors from the Type String to Player.

OK, I also needed to change the types of the fields, of course, and the type of the ScoreCard.point method parameter.

Now it compiles again - let's see wether the tests run. They do. Good. Let's commit.

[Ilja Preuss]

Now we actually can get rid again of the playerOne and playerTwo fields in ScoreCard and TennisContest. Let's start with TennisContest:

    public TennisContest(Player playerOne, Player playerTwo) {
        reset();
    }
...
    protected void reset() {
        score.put(Player.ONE, new Integer(0));
        score.put(Player.TWO, new Integer(0));
    }

As we are already touching the reset method, we can also make use of autoboxing:

    protected void reset() {
        score.put(Player.ONE, 0);
        score.put(Player.TWO, 0);
    }

Tests still run. Let's get rid of the constructor arguments.

    public TennisContest() {
        reset();
    }

We can also remove them from TennisGame and TennisSet. In fact, for TennisSet, the default constructor suffices now. And really like to delete code!

public TennisGame(GameListener listener) {
setGameListener(listener);
}
 
public TennisGame() {
this(null);
}

    public ScoreCard(Player playerOne, Player playerTwo) {
    this.playerOne = playerOne;
    this.playerTwo = playerTwo;
        game = new TennisGame();
        set = new TennisSet();
        game.setGameListener(set);
    }

Tests still run! Now we do the same to ScoreCard.

Again, tests still run. Again, time to commit.

[Ilja Preuss]

Now let's take a look at an example of a game test:

public void testPointProgression2() {
game.point(Player.TWO);
assertEquals(1, game.getScore(Player.TWO));
assertEquals(null, listener.winner());
game.point(Player.TWO);
assertEquals(2, game.getScore(Player.TWO));
assertEquals(null, listener.winner());
game.point(Player.TWO);
assertEquals(3, game.getScore(Player.TWO));
assertEquals(null, listener.winner());
game.point(Player.TWO);
assertEquals(Player.TWO, listener.winner());
}

I like that better than before, but your mileage may vary. Anyway, there is still some duplication left.

For example, take the line

assertEquals(null, listener.winner());

which is repeated over and over again. I will extract it into an assertNoGameWinner() method.

public void testPointProgression2() {
game.point(Player.TWO);
assertEquals(1, game.getScore(Player.TWO));
assertNoGameWinner();
game.point(Player.TWO);
assertEquals(2, game.getScore(Player.TWO));
assertNoGameWinner();
game.point(Player.TWO);
assertEquals(3, game.getScore(Player.TWO));
assertNoGameWinner();
game.point(Player.TWO);
assertEquals(Player.TWO, listener.winner());
}
 
private void assertNoGameWinner() {
assertEquals(null, listener.winner());
}

Let's also see what we can do with those lines like

assertEquals(1, game.getScore(Player.TWO));

public void testPointProgression2() {
game.point(Player.TWO);
assertGameScoreEquals(1, Player.TWO);
assertNoGameWinner();
game.point(Player.TWO);
assertGameScoreEquals(2, Player.TWO);
assertNoGameWinner();
game.point(Player.TWO);
assertGameScoreEquals(3, Player.TWO);
assertNoGameWinner();
game.point(Player.TWO);
assertEquals(Player.TWO, listener.winner());
}
 
private void assertGameScoreEquals(int expectedScore, Player player) {
assertEquals(expectedScore, game.getScore(player));
}

I like to refactor my tests in these ways - if we later change the way our production class works, we will likely have much less work to do adjusting our tests.

For reasons of symmetry, let's also extract that last assert statement.

assertGameWinner(Player.TWO);
}
 
private void assertGameWinner(Player expectedWinner) {
assertEquals(expectedWinner, listener.winner());
}

We should also adjust the other test, of course.

public void testPointProgression() {
game.point(Player.ONE);
assertGameScoreEquals(1, Player.ONE);
assertNoGameWinner();
game.point(Player.ONE);
assertGameScoreEquals(2, Player.ONE);
assertNoGameWinner();
game.point(Player.ONE);
assertGameScoreEquals(3, Player.ONE);
assertNoGameWinner();
game.point(Player.ONE);
assertGameWinner(Player.ONE);
}

Tests still run. Now it's *really* time to write a new test. Remember? We wanted to implement "deuce"...

[Ilja Preuss]

So let's write a unit test for "deuce".

public void testDeuce() {
game.point(Player.TWO);
game.point(Player.TWO);
game.point(Player.TWO);
game.point(Player.ONE);
game.point(Player.ONE);
game.point(Player.ONE);
assertTrue(game.isDeuce());
assertNoGameWinner();
}

As you can see, I decided that the game would know whether it is in a "deuce" state. I also made sure that a deuce means that there is no winner yet. I assume that once we are in a deuce state, we don't care about the player's actual scores anymore, so I don't test for it.

Of course this doesn't compile yet. We need to creat that isDeuce method.

public boolean isDeuce() {
// TODO Auto-generated method stub
return false;
}

Now the tests should fail. And they do.

Making the test pass is easy:

public boolean isDeuce() {
return getScore(Player.ONE) == getScore(Player.TWO);
}

Yes, that's right - all tests pass! But that's wrong, isn't it? We need more tests...

public void testNoDeuceBefore40_40() {
game.point(Player.TWO);
game.point(Player.ONE);
assertFalse(game.isDeuce());
game.point(Player.TWO);
game.point(Player.ONE);
assertFalse(game.isDeuce());
game.point(Player.TWO);
game.point(Player.ONE);
assertFalse(game.isDeuce());
}

That test fails and forces us to correct the production code.

public boolean isDeuce() {
return getScore(Player.ONE) > 3
&& getScore(Player.ONE) == getScore(Player.TWO);
}

Ouch, that fails. I guess the "3" is wrong.

Yes, the testDeuce fails, so it still thinks there is no deuce. That should probably be a "2".

Now the testNoDeuceBefore40_40 fails. The last block actually already is a 40-40. Stupid me...

public void testNoDeuceBefore40_40() {
game.point(Player.TWO);
game.point(Player.ONE);
assertFalse(game.isDeuce());
game.point(Player.TWO);
game.point(Player.ONE);
assertFalse(game.isDeuce());
}

Now the tests run all green. Phew. Time to commit...

[Ilja Preuss]

Now TennisGame knows about Deuce. Seems to me the only thing missing is the ScoreCard knowing about it, too. Let's write a test for that.

We don't yet have a ScoreCardTest - shame on us. Well, it's never to late to start...

package tests;
 
import org.javaranch.ScoreCard;
 
import junit.framework.TestCase;
 
public class ScoreCardTest extends TestCase {
 
public void testDeuce() throws Exception {
ScoreCard card = new ScoreCard();
|
}

}

Mhh, I'd actually prefer not to have to code all the calls to the point method just to test that the ScoreCard knows how to show a Deuce. I'd like to mock the game, but for that, we have to pass the game into the score card again. Let's change that first. We can also remove the point method from the ScoreCard again.

Tests run. Now we need to introduce an interface for the TennisGame to be able to mock it. It only needs to contain the methods getScore(Player) and isDeuce(), because that's all the ScoreCard needs to know. TennisGameScore might be a good name. I create the interface by using the "Extract Interface" refactoring that Eclipse provides.

package org.javaranch;
 
public interface TennisGameScore {
 
boolean isDeuce();
 
int getScore(Object player);
 
}

public class TennisGame extends TennisContest implements TennisGameScore {

Now we can finally write that test:

public void testDeuce() throws Exception {
TennisGameScore deuceGameScore = new TennisGameScore() {
public boolean isDeuce() {
return true;
}
 
public int getScore(Object player) {
throw new AssertionError("should not be called");
}
 
};
ScoreCard card = new ScoreCard(deuceGameScore, new TennisSet());
assertEquals("Deuce", card.scoreGame());
}

It fails as expected, because the getScore method gets called.

Let's correct the implementation:

public String scoreGame() {
if (game.isDeuce()) {
return "Deuce";
}
return display(game.getScore(Player.ONE)) + "-"
+ display(game.getScore(Player.TWO));
}

Tests run!

It's time to activate that acceptance test again.

That one passes, too! Success! And enough for today...

[Frank Carver]

I read through those last few posts with a wry smile. It seems as if we each have a different "vision" for where the design is going, and it "flips" and "flops" each time we hand over.

If were were in the same place, I'd definately want to spend some time now discussing class responsibilities. I think we have slightly different ideas about what each class is for. I don't usually use CRC cards much myself, but this seems like a good occasion to use them.

Can you think of a good way to do something like this in the asynchronous way we are currently working?

Ilja wrote: One thing that bugs me are the player strings all over the place. In fact I think those shouldn't be strings at all - I prefer it to use the compilers ability to find my typing errors.

Ha! That'll teach me to proceed using Strings without driving it with a test. I even had such a test in mind, but I got so caught up in the process and writing about it that I never added that one.

I'll admit that I was also feeling the itch of the same String text appearing lots of times. My approach, however, would have probably been a smaller step - such as to note that they only appear in the test cases, and use a simple "extract constant" refactoring to get the compiler on my side

Now I'll synchronise code and take a good look at where it has got to, then I'll come back with a response later.

[Frank Carver]

OK. I hadn't really thought about it before, but I reckon one of the reasons I don't use enums much is because they evoke in me the same sort of unease as Singletons. Hard-coding an enum reference deep in a call hierarchy ties things too closely for my liking. I guess that's where I am "extreme".

Anyway. Given that the unwritten rules of this game seem to be that you have to work with what you are given, my first step is to localize the use of the Player enum. A quick check using Eclipse's reference finder shows that Player enum is used (not including the tests) in ScoreCard, TennisContest, and TennisGame. I'll try and limit it to ScoreCard for the moment.

The use in TennisContest is a bit smelly anyway. It currently does:

    protected void reset() {
        score.put(Player.ONE, 0);
        score.put(Player.TWO, 0);
    }

I can replace this with a slightly more wordy, but more generic, solution:

    protected void reset() {
        Iterator it = score.keySet().iterator();
        while (it.hasNext()) {
            score.put(it.next(), 0);
        }
    }

As Ilja has already "raised the bar" and added a dependency on Java 5, let's use the considerably neater "for-each" syntax:

    protected void reset() {
        for(Object key : score.keySet()) {
            score.put(key, 0);
        }
    }

That looks good, but suddenly almost all the tests fail. The reason? The initial call to "reset" in the TennisContest constructor is looping round an empty Map, and resetting no scores. Whoops.

Now this is a bit tricky. My aim for this refactoring is to remove dependencies on Player from TennisContest, so I can't hard-code Player.ONE and Player.TWO in the constructor. Even though my natural inclination would be to pass in the player objects, that doesn't seem to be where Ilja wants to go, so let's look at alternatives.

Currently the TennisContest constructor looks like:

    private Map score = new HashMap();
 
    public TennisContest() {
        reset();
    }

A few possibilities occur to me:

Pass in some sort of list of players, and insert them into the Map

pass in a pre-populated Map object and use it as-is

pass in a pre-populated "prototype" map, and copy it

The first option is not much different from passing the players in individually. The second runs the risk of accidentally using the same map for two objects, so I plump for the third:

    private HashMap score;
 
    public TennisContest(HashMap prototype) {
        this.score = (HashMap)prototype.clone();
        reset();
    }

The downside is that we now pass in a specific type of Map, which I don't usually like to do.

Now, I'd love to be able to use Java's Cloneable interface here, but (IMHO) Java's type system is screwed up around the operation of the "clone" method. So even though HashMap implements "Cloneable" and "Map", and has a working "clone" method, it can't be used in this case. Cloneable has no methods, not even "clone()". huh?

So let's create a small new interface, CloneableMap which extends Map with a "clone" method:

import java.util.Map;
 
public interface CloneableMap extends Map {
    Object clone();
}

The TennisContest constructor now looks like:

    private Map score;
 
    public TennisContest(CloneableMap prototype) {
        this.score = (Map)prototype.clone();
        reset();
    }

This has, of course, broken the classes which extend TennisContest, TennisGame and TennisSet. The TennisGame constructors look like:

    public TennisGame(GameListener listener) {
        setGameListener(listener);
    }
 
    public TennisGame() {
        this(null);
    }

Let's just pass through the CloneableMap:

    public TennisGame(CloneableMap players, GameListener listener) {
        super(players);
        setGameListener(listener);
    }
 
    public TennisGame(CloneableMap players) {
        this(players, null);
    }

TennisSet currently has no explicit constructors, so let's add one:

    public TennisSet(CloneableMap players) {
        super(players);
    }

Now the domain classes all compile, but several of the tests don't. All of them fail because they are calling the no-arg constructors of TennisGame and TennisSet.

Now, I could create some sort of player map objects in each of these tests, but that seems needless duplication. All of the tests are testing the same sort of tennis games, so why not share a base class between the tests:

package tests;
 
import org.javaranch.CloneableMap;
 
import junit.framework.TestCase;
 
public class TwoPlayerTestCase extends TestCase {
    protected CloneableMap players;
}

Now all we need to do is to make all our test case classes inherit from this one, and pass the "players" field to the TennisGame and TennisSet constructors.

The whole lot now compiles again, but I know that when I run the tests there will be a problem. And I'm right. Now all the tests fail. Because I have not initialised that "players" object.

Now, I'd love to be able to just assign a HashMap to it, but HashMap doesn't implement our new CloneableMap interface, even though it has all the methods - we need another tiny adapter class:

package org.javaranch;
 
import java.util.HashMap;
 
public class PlayerMap extends HashMap implements CloneableMap {
}

Now we can add it bit of initialisation code to TwoPlayerTestCase:

public class TwoPlayerTestCase extends TestCase {
    protected CloneableMap players = new PlayerMap();
    {
        players.put(Player.ONE, null);
        players.put(Player.TWO, null);
    }
}

And everything works again. All the tests pass.

Phew. Check it in.

[Ilja Preuss]

Originally posted by Frank Carver:
I read through those last few posts with a wry smile. It seems as if we each have a different "vision" for where the design is going

Not only that, but actually quite different preferences of style. But that is part of what makes it so interesting...

and it "flips" and "flops" each time we hand over.

Yes, but also partly because I like to experiment. And I don't think it really flops back into the same position - the design really isn't like I envisioned it, I've already learned a lot from your ideas.

Of course this would be much easier if we were sitting in the same room...

If were were in the same place, I'd definately want to spend some time now discussing class responsibilities. I think we have slightly different ideas about what each class is for. I don't usually use CRC cards much myself, but this seems like a good occasion to use them.

Yes. I've recently heard about cardmeeting.com, but it seems to be down right now...

I'll admit that I was also feeling the itch of the same String text appearing lots of times. My approach, however, would have probably been a smaller step - such as to note that they only appear in the test cases, and use a simple "extract constant" refactoring to get the compiler on my side

I thought about that, but somehow using Strings felt inadequate to me. I guess that's one of the basic difference of our styles - I like the expressiveness of explicite classes, whereas you seem to prefer the flexibility of "primitives"?

I'm sure we will find a good mix.

[Ilja Preuss]

Originally posted by Frank Carver:
OK. I hadn't really thought about it before, but I reckon one of the reasons I don't use enums much is because they evoke in me the same sort of unease as Singletons. Hard-coding an enum reference deep in a call hierarchy ties things too closely for my liking. I guess that's where I am "extreme".

Mhh, to me, enums really are like typed constants. And I'm not that much concerned about using them all over the code in this case. After all, it is hardcoded all over the place that we are dealing with exactly two sides (the whole deuce idea wouldn't make sense otherwise). I don't expect it to make much of a difference what objects we use as keys, so I'm not troubled by making the keys globally known. But probably I'm missing something - it wouldn't be the first time I had to "unfactor" a design decision.

[Ilja Preuss]

I checked in the .settings folder of the eclipse project, so that we use the same formatting etc. Change it as you like - it's just important to me that I use the same settings.

[Ilja Preuss]

TennisGame.isDeuce still references the Player enum:

    public boolean isDeuce() {
        return getScore(Player.ONE) > 2
                && getScore(Player.ONE) == getScore(Player.TWO);
    }

Frank, any idea how you want to tackle that?

[Frank Carver]

OK, that has removed the Player dependency from TennisSet. Now, can we do anything with TennisGame?

The only place that Player is referenced in TennisGame is in the "isDeuce" method:

    public boolean isDeuce() {
        return getScore(Player.ONE) > 2
                && getScore(Player.ONE) == getScore(Player.TWO);
    }

Now, I need to declare an opinion here. Personally I feel quite strongly that this method (and especially the tests that call it) are the wrong way to go.

Back when I was thinking about how to deal with a game win, I deliberately rejected the idea of an accessor method on TennisGame to fetch the winner. This sort of polling "pull" functionality seems completely at odds with the way a tennis game actually works. I really don't see any justification for client code to keep asking the TennisGame what its status is, or for the TennisGame to expose such details of its internal workings.

Driving the creation of this method with tests is (for me, at least) an example of how test-driven developement can sometimes drive poor designs as well as good ones. It's all too easy to quickly create a test, then create an implementation which matches it, even though neither add much value to the solution. Worst of all though, enshrining the use of such a method in the test cases makes it seem like a real customer requirement, and makes it much too "solid" and hard to change.

Anyway. Rant over. Now to "fix" it

Let's consider some possibilities:

add a "callback" like we did for the winner

move the "deuce" decision to ScoreCard

introduce a new class to manage "deuce" processing

This is a tough decision. In many ways it comes down to class responsibilities as I mentioned above. Here's how I see the class responsibilities at the moment, even though I feel that Ilja may have a different take.

The job of TennisContest is to provide common behaviour between the "contest" classes TennisGame, TennisSet, and a potential TennisMatch.

The job of the "contest" classes is to manage the score of a particular contest, including notifying anyone interested of a win condition.

The current job of ScoreCard is to manage the overall scenario, including "wiring" together contest classes and player definitions and acting as a "facade" to isolate clients from implementation details, This class also has the job of displaying the scores in a textual format.

Written like that, ScoreCard seems heavy compared with the others. It has too many different responsibilities.

I can see the potential for three separate class roles here. One to manage the scenario and wiring up the collaborators. One to act as a client facade, and one to format output.

It seems that my steps in this refactoring game have tended toward leaving ScoreCard with the facade and scenario roles, whereas Ilja's steps have tended toward leaving ScoreCard with just the formatting roles, and kicking the facade and scenario roles back into the tests.

I feel quite strongly that the functional tests should not need to know about the implementation of game, set, match and player objects, and how they are wired together. I want to see functional tests which just say "create a tennis competition between these two players. here is a sequence of points scored, what's the score card?" This was, after all, the nearest thing to a customer specification that we had, at the start of this thread.

For this to work we need some sort of facade, but this need not be the current ScoreCard class.

It has changed several times, but the current ScoreCard is almost all formatting. So let's change its name to TennisFormatter.

Now let's rewrite the functional tests in terms of a facade class and see what we need to make to hook that up with the existing implementation.

The current "setUp" method in the functional tests looks like:

    private TennisFormatter scoreCard;
    private TennisGame game;
 
    public void setUp() {
        game = new TennisGame(players);
        TennisSet set = new TennisSet(players);
        scoreCard = new TennisFormatter(game, set);
        game.setGameListener(set);
    }

I want to replace that with somehing much simpler. How about:

    private Tennis tennis;
 
    public void setUp() {
        tennis = new Tennis("Jimmy Connors", "John McEnroe");
    }

To make this compile, we need to create a Tennis class with the right constructor. Luckily Eclipse can do this for us. Then we just need to paste in the old members and set up code from the fuctional test:

public class Tennis {
    private TennisFormatter scoreCard;
    private TennisGame game;
 
    public Tennis(String name1, String name2) {
        game = new TennisGame(players);
        TennisSet set = new TennisSet(players);
        scoreCard = new TennisFormatter(game, set);
        game.setGameListener(set);
    }
}

Hmm. Now this wonn't compile because it is using the "players" map we set up in the test case base class. Let's copy over the setup from there for now:

public class Tennis {
    private TennisFormatter scoreCard;
    private TennisGame game;
 
    public Tennis(String name1, String name2) {
        CloneableMap players = new PlayerMap();
        players.put(Player.ONE, null);
        players.put(Player.TWO, null);
        game = new TennisGame(players);
        TennisSet set = new TennisSet(players);
        scoreCard = new TennisFormatter(game, set);
        game.setGameListener(set);
    }
}

There are things about that which I don't like, but my main job at the moment is to get things compiling again, so I'll defer these changes.

Now that looks much more like a functional test set up to me. It breaks a lot of tests. Let's fix them one by one.

    public void testNoWinsNoScore() {
        assertEquals("0-0 0-0 0-0", scoreCard.fullCard());
    }

This one won't compile because we have no "scoreCard" member any more. Let's provide a "scoreCard" method on our Tennis facade:

    public Object scoreCard() {
        return scoreCard.fullCard();
    }

and search/replace all the functional tests to use it:

    public void testNoWinsNoScore() {
        assertEquals("0-0 0-0 0-0", tennis.scoreCard());
    }

Getting better, but the next test case is still broken, too:

    public void testOneWinPlayerOne() {
        game.point(Player.ONE);
        assertEquals("0-0 0-0 15-0", tennis.scoreCard());
    }

There is no "game" object. Let's add a "point" method to our tennis facade:

    public void point(Object player) {
        game.point(player);
    }

and find/replace as before to use it in the functional test case:

    public void testOneWinPlayerOne() {
        tennis.point(Player.ONE);
        assertEquals("0-0 0-0 15-0", tennis.scoreCard());
    }

Excellent, it all now compiles again, and all the tests pass. I'll check in the changes, and continue with the next step in the next post.

[Frank Carver]

Ilja wrote Frank, any idea how you want to tackle that?

Looks like we overlapped a little. These descriptive posts take so darn long to write.

As for the formatting, I was getting fed up with tabbing looking awful, so I opted for four-spaces and reformatted the whole project. I'm sure the original problem was at my end as I have been doing this on two different machines with two different sets of Eclipse defaults. It shouldn't happen again.

Anyway, I've still got a bit more to go before I hand over this chunk, so I'll get back to it.

[Ilja Preuss]

Here is a little refactoring:

As I said earlier, the use of the Map in TennisContest can be simplified a little bit by using generics (plus autoboxin):

    private Map<Object, Integer> score;
 
    public int getScore(Object player) {
        return score.get(player);
    }
 
    public void point(Object player) {
        score.put(player, score.get(player) + 1);
    }

Another thing we can make use of are covariant return types in CloneableMap:

public class PlayerMap extends HashMap<Object, Integer> implements CloneableMap<Object, Integer> {
 
    @SuppressWarnings("unchecked")
    @Override
    public PlayerMap clone() {
        return (PlayerMap) super.clone();
    }
    
}

This simplifies the call to clone() in TennisContest to

    public TennisContest(CloneableMap<Object, Integer> prototype) {
        this.score = prototype.clone();
        reset();
    }

I'm not sure whether you will like it, Frank, but somehow I do...

OK, now I will go to bed, before I wreak more havoc...

[Ilja Preuss]

Oops, sorry for the overlap - guess I was a little bit to enthusiastic.

If you have trouble merging my changes, don't hesitate to just override them.

[Frank Carver]

I'm trying to commit some of my changes and getting the following:

commit -m "introduced new Tennis facade to align ScoreCard concepts
updated functional tests to use new facade" E:/projects/tennis2/TennisMatch/org/javaranch/ScoreCard.java E:/projects/tennis2/TennisMatch/org/javaranch/Tennis.java E:/projects/tennis2/TennisMatch/org/javaranch/TennisFormatter.java E:/projects/tennis2/TennisMatch/tests/ScoreCardTest.java E:/projects/tennis2/TennisMatch/tests/TennisFunctionalTest.java
Deleting E:/projects/tennis2/TennisMatch/org/javaranch/ScoreCard.java
Adding E:/projects/tennis2/TennisMatch/org/javaranch/Tennis.java
Adding E:/projects/tennis2/TennisMatch/org/javaranch/TennisFormatter.java
svn: Commit failed (details follow):
svn: Commit failed (details follow):
svn: COPY of '/svnroot/jfcfixtures/!svn/bc/13/JavaRanch_TennisMatch/trunk/org/javaranch/ScoreCard.java': HTTP/1.1 502 Bad Gateway ('https://svn.sourceforge.net')

This is strange, because I can do other svn operations (I updated your changes, and committed some other files, for example)

Have you ever seen this sort of thing before?

[Ilja Preuss]

Originally posted by Frank Carver:
The only place that Player is referenced in TennisGame is in the "isDeuce" method:

    public boolean isDeuce() {
        return getScore(Player.ONE) > 2
                && getScore(Player.ONE) == getScore(Player.TWO);
    }

Now, I need to declare an opinion here. Personally I feel quite strongly that this method (and especially the tests that call it) are the wrong way to go.

Frankly, I didn't see it as the final solution - actually I expected it to be a temporary one. I mainly used it as the "simplest thing that could possibly work" to drive the "deuce algorithm".

Back when I was thinking about how to deal with a game win, I deliberately rejected the idea of an accessor method on TennisGame to fetch the winner.

Which was an interesting idea I liked quite a lot!

This sort of polling "pull" functionality seems completely at odds with the way a tennis game actually works.

I can see how it was a good solution for "game winner". The deuce thing looks a bit different to me - it doesn't really depend on how tennis works in general, but whether our scoring display will work "push" or "pull".

I really don't see any justification for client code to keep asking the TennisGame what its status is, or for the TennisGame to expose such details of its internal workings.

I guess we need to make it a real, usable application to decide that. We probably should have done that much earlier. If you like, I could try that next when it is my turn...

Driving the creation of this method with tests is (for me, at least) an example of how test-driven developement can sometimes drive poor designs as well as good ones. It's all too easy to quickly create a test, then create an implementation which matches it, even though neither add much value to the solution. Worst of all though, enshrining the use of such a method in the test cases makes it seem like a real customer requirement, and makes it much too "solid" and hard to change.

With all due respect, without a true client, your decision to *not* use a getter sounds as arbitrary to me. But I'm not very troubled by it - I don't yet feel that it's hard to change at all...

The job of TennisContest is to provide common behaviour between the "contest" classes TennisGame, TennisSet, and a potential TennisMatch.

The job of the "contest" classes is to manage the score of a particular contest, including notifying anyone interested of a win condition.

Same here so far.

It seems that my steps in this refactoring game have tended toward leaving ScoreCard with the facade and scenario roles, whereas Ilja's steps have tended toward leaving ScoreCard with just the formatting roles, and kicking the facade and scenario roles back into the tests.

That sounds about right, yes.

For this to work we need some sort of facade, but this need not be the current ScoreCard class.

I'm not sure that we really need that facade, or that it needs to be in the production code, but we will see...

It has changed several times, but the current ScoreCard is almost all formatting. So let's change its name to TennisFormatter.

Mhh, I think ScoreCard was just fine. What else is a score card then the formatted view of the current score of a match?

[Ilja Preuss]

Originally posted by Frank Carver:

Have you ever seen this sort of thing before?

If so, I don't remember.

From http://www.checkupdown.com/status/E502.html it sounds as if you can't do much but try again later...

[Ilja Preuss]

Originally posted by Frank Carver:

I want to replace that with somehing much simpler. How about:

    private Tennis tennis;
 
    public void setUp() {
        tennis = new Tennis("Jimmy Connors", "John McEnroe");
    }

Frank, I don't think we have any requirement for actual player names from our customer, do we?

[Frank Carver]

Hmm. I'm doing three things at once here. In one window I have the description of my next changes, which (hopefully) should answer some of this stuff. In this window I'm discussing philosophical issues, and in Eclipse I'm swearing at subversion

My leaning toward naming ScoreCard as ScoreFormatter is solely to make its job clearer. For a long while, all ScoreCard said to me was "facade". I had a mental image of an "electronic score card" running on a PDA and acting like a smart version of a paper form. The user starts it up, enters the player names, then taps a "hot spot" appropriate to the player each time a point is scored. Each time a point is tapped, the overall score is updated on the screen.

My understanding may be aligned, or at odds, with the original customer's wishes, but all I can be sure of at the moment, is that my understanding differs from yours. So I thought I'd make it clearer by separating the two concepts and creating a class for each.

I reckon it's much harder to argue about the responsibilities if a "score formatter" than a more abstract "score card".

As a small teaser, I also have in mind the idea that there are several different ways of formatting tennis scores, so I want to make sure our design doesn't make it too hard to add pluggable behaviour for this later.

As for the subversion problems I'm getting very fed up with that. I may just push on with my next changes, and try to sort out the commit issues when I've finished.

[Frank Carver]

That last post was a bit of detour, but I think it might have helped clear up the question which prompted it. Where shall we put the "deuce" processing?

The new "lite" ScoreCard (a.k.a TennisFormatter) class seems the logical place to me. Its job is all about formatting, and interpreting three pointe each (or four points each, or whatever) as "deuce" sounds like formatting to me.

So let's leave the TennisGame class just responsible for counting scores and determining a win:

public class TennisGame extends TennisContest implements TennisGameScore {
 
    public TennisGame(CloneableMap players, GameListener listener) {
        super(players);
        setGameListener(listener);
    }
 
    public TennisGame(CloneableMap players) {
        this(players, null);
    }
 
    public void point(Object player) {
        if (getScore(player) == 3) {
            reset();
            notifyWinner(player);
        } else {
            super.point(player);
        }
    }
}

I know the victory condition stuff is still wrong, but we can add tests to drive that later.

Let's just move the old method to TennisFormatter and tweak the method recievers a little to make it work.

    public String scoreGame() {
        if (isDeuce()) {
            return "Deuce";
        }
        return display(game.getScore(Player.ONE)) + "-"
                + display(game.getScore(Player.TWO));
    }
 
    public boolean isDeuce() {
        return game.getScore(Player.ONE) > 2
                && game.getScore(Player.ONE) == game.getScore(Player.TWO);
    }

To me, it looks much more at home here.

To make this work, I'm going to unroll Ilja's TennisGameScore changes. These were all there to support the test in ScoreCardTest so I'll begin by commenting that out. I'll also comment out the tests for the deuce behaviour in TennisGame Test. I'm expecting that eventually something like them will end up in ScoreCardTest (or TennisFormatterTest as it probably ought to be)

Then I'll go though replacing TennisGameScore with TennisGame again.

TennisGameScore is now no longer referenced, so we can remove it.

A quick check. All tests pass except a warning from the one we have commented out.

So let's change the name to TennisFormatterTest and bring it back.

public class TennisFormatterTest extends TwoPlayerTestCase {
 
    public void testDeuce() throws Exception {
        TennisGameScore deuceGameScore = new TennisGameScore() {
            public boolean isDeuce() {
                return true;
            }
 
            public int getScore(Object player) {
                throw new AssertionError("should not be called");
            }
 
        };
        TennisFormatter card = new TennisFormatter(deuceGameScore, new TennisSet(players));
        assertEquals("Deuce", card.scoreGame());
    }
 
}

Obviously this fails to compile. TennisGameScore has gone. TennisFormatter takes a TennisGame as a parameter so we need to pass one in. What we don't want to do is duplicate all the steps to get to a deuce. We need a TennisGame and a way to "poke" scores directly in. Sounds like a subclass to me:

lass ShortcutTennisGame extends TennisGame {
    public ShortcutTennisGame(CloneableMap<Object, Integer> players) {
        super(players);
    }
 
    public void inject(int score1, int score2) {
        score.put(Player.ONE, score1);
        score.put(Player.TWO, score2);
    }
}
 
public class TennisFormatterTest extends TwoPlayerTestCase {
 
    ShortcutTennisGame game;
    TennisFormatter formatter;
    
    public void setUp() {
        game = new ShortcutTennisGame(players);
        formatter = new TennisFormatter(game, null);
    }
    
    public void testDeuce() throws Exception {
        game.inject(3,3);
        assertEquals("Deuce", formatter.scoreGame());
    }
}

That almost works. It just needs the "score" array in TennisContest to be marked as protected.

Good. All tests pass. Now to add a few more "deuce" tests:

    public void testDeuce3() throws Exception {
        assertEquals("0-0", formatter.scoreGame());
        game.inject(3,3);
        assertEquals("Deuce", formatter.scoreGame());
    }
 
    public void testDeuce4() throws Exception {
        assertEquals("0-0", formatter.scoreGame());
        game.inject(4,4);
        assertEquals("Deuce", formatter.scoreGame());
    }
 
    public void testDeuce5() throws Exception {
        assertEquals("0-0", formatter.scoreGame());
        game.inject(5,5);
        assertEquals("Deuce", formatter.scoreGame());
    }
        
    public void testDeuce99() throws Exception {
        assertEquals("0-0", formatter.scoreGame());
        game.inject(99,99);
        assertEquals("Deuce", formatter.scoreGame());
    }

They work OK, but there's a lot of duplication. So let's "extract method":

    private void _testDeuce(int score) {
        assertEquals("0-0", formatter.scoreGame());
        game.inject(score, score);
        assertEquals("Deuce", formatter.scoreGame());
    }
 
    public void testDeuce3() throws Exception {
        _testDeuce(3);
    }
 
    public void testDeuce4() throws Exception {
        _testDeuce(4);
    }
 
    public void testDeuce5() throws Exception {
        _testDeuce(5);
    }
        
    public void testDeuce99() throws Exception {
        _testDeuce(99);
    }

That's better.

Remember back at the start of this session, my aim was to rationalise the use of the Player enum? Well, it's still used in TennisFormatter, but I'm not just going to change it, I'm going to drive the next bit from a test for real user-level functionality.

Let's add another test to TennisFormatterTest:

    public void testAdvantage() {
        assertEquals("0-0", formatter.scoreGame());
        game.inject(3, 4);
        assertEquals("Advantage McEnroe", formatter.scoreGame());
    }

Guess what, it fails with "expected <Advantage McEnroe> but was <40-null>". generating the advantage bit is pretty straighforward now we have done deuce:

    public String scoreGame() {
        if (isDeuce()) {
            return "Deuce";
        }
        if (isAdvantage()) {
            return "Advantage";
        }
        return display(game.getScore(Player.ONE)) + "-"
                + display(game.getScore(Player.TWO));
    }
 
    private boolean isAdvantage() {
        return game.getScore(Player.ONE) > 2 && game.getScore(Player.ONE) > 2 
                && (game.getScore(Player.ONE) == game.getScore(Player.TWO) + 1 
                        || game.getScore(Player.TWO) == game.getScore(Player.ONE) + 1);
    }

OK, that boolean expression looks a bit messy, but we can sort that out later. The most important thing is that the "Advantage" text is correct and the test only fails because the player name is not shown.

Somehow we need to get that name. But in order to do so, we need to know which player has the advantage. So let's change the return type of the isAdvantage method to Object:

   public String scoreGame() {
        if (isDeuce()) {
            return "Deuce";
        }
        Object advantage = isAdvantage(); 
        if (advantage != null) {
            return "Advantage " + advantage;
        }
        return display(game.getScore(Player.ONE)) + "-"
                + display(game.getScore(Player.TWO));
    }
 
    private Object isAdvantage() {
        if (game.getScore(Player.ONE) <= 2 || game.getScore(Player.ONE) <= 2) {
            return null;
        }
        if (game.getScore(Player.ONE) == game.getScore(Player.TWO) + 1) {
            return Player.ONE;
        }
        if (game.getScore(Player.TWO) == game.getScore(Player.ONE) + 1) {
            return Player.TWO;
        }
        return null;
    }

Now we are getting closer. It now gives "Advantage ONE". Now we just need to return the correct player name. We don't have access to them in this class, so let's pass them in to the constructor for now:

    private String player1;
    private String player2;
 
    public TennisFormatter(String player1, String player2, TennisGame tennisGame, TennisSet tennisSet) {
        this.player1 = player1;
        this.player2 = player2;
        game = tennisGame;
        set = tennisSet;
    }

Now we can use these variables instead of Player.ONE and Player.TWO in the advantage code:

    private Object isAdvantage() {
        if (game.getScore(Player.ONE) <= 2 || game.getScore(Player.ONE) <= 2) {
            return null;
        }
        if (game.getScore(Player.ONE) == game.getScore(Player.TWO) + 1) {
            return player1;
        }
        if (game.getScore(Player.TWO) == game.getScore(Player.ONE) + 1) {
            return player2;
        }
        return null;
    }

and a few small fixes to the constructor calls in TennisFormatterTest:

        formatter = new TennisFormatter("Connors", "McEnroe", game, null);

and Tennis:

    public Tennis(String name1, String name2) {
        CloneableMap<Object, Integer> players = new PlayerMap();
        players.put(Player.ONE, null);
        players.put(Player.TWO, null);
        game = new TennisGame(players);
        TennisSet set = new TennisSet(players);
        scoreCard = new TennisFormatter(name1, name2, game, set);
        game.setGameListener(set);
    }

All tests pass once again.

Personally, I still feel that there is a certain amount of duplication between the Player enum and the player names used in the formatter, but I can live with that for the moment. I really feel that it is time I let go of the virtual keyboard for a while.

I still can't get my changes to commit to subversion, so if you wish to continue with this, you can download a snapshot from http://www.servletshop.com/downloads/TennisMatch.zip

[Garrett Rowe]

I have to say I love this thread, especially the metaphor of a tennis match juxtaposed against the back and forth between Ilja and Frank.