Request For Comments: TennisGameTest Refactoring

[Junilu Lacar]

Just wanted to get some other people's thoughts about a refactoring I just did. I wrote this test in reply to another recent thread. The problem was to write a program to score a tennis game.

This was the test BEFORE I did some major refactoring to make better use of lambdas and functional interfaces. The parts you may want to focus on are the @BeforeEach setup, the @FunctionalInterface declared at the bottom, and the winPoints() helper method. Those are the main things that changed in the AFTER refactoring version. The AFTER version is a bit longer only because I added another nested test class for exceptions.

import org.junit.jupiter.api.*;

import static org.junit.jupiter.api.Assertions.*;

@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)
class TennisGameTest {

    private TennisGame game;

    private Procedure server;
    private Procedure receiver;

    @BeforeEach
    void setUp() {
        game = new TennisGame();
        server = () -> game.serverWinsPoint();
        receiver = () -> game.receiverWinsPoint();
    }

    @Nested
    class Basic_Scoring {

        @Test
        void it_starts_at_love_all() {
            assertEquals("Love all", game.getScore());
        }

        @Test
        void it_gives_server_score_first() {
            game.serverWinsPoint();
            assertEquals("15 - Love", game.getScore());

            game.serverWinsPoint();
            assertEquals("30 - Love", game.getScore());

            game.serverWinsPoint();
            assertEquals("40 - Love", game.getScore());
        }

        @Test
        void it_gives_receiver_score_second() {
            game.receiverWinsPoint();
            assertEquals("Love - 15", game.getScore());

            game.receiverWinsPoint();
            assertEquals("Love - 30", game.getScore());

            game.receiverWinsPoint();
            assertEquals("Love - 40", game.getScore());
        }

        @Test
        void it_uses_all_when_score_is_tied() {
            game.receiverWinsPoint();
            game.serverWinsPoint();
            assertEquals("15 all", game.getScore());

            game.receiverWinsPoint();
            game.serverWinsPoint();
            assertEquals("30 all", game.getScore());
        }
    }

    @Nested
    class Basic_Win {
        @Test
        void server_wins_with_four_points() {
            winPoints(server, 4);

            assertEquals("Server wins", game.getScore());
        }

        @Test
        void receiver_wins_with_four_points() {
            winPoints(receiver, 4);

            assertEquals("Receiver wins", game.getScore());
        }

        @Test
        void at_40_30_server_wins_game_with_next_point() {
            winPoints(receiver, 2);
            winPoints(server, 3);    // 40-30

            game.serverWinsPoint();

            assertEquals("Server wins", game.getScore());
        }

        @Test
        void at_30_40_receiver_wins_game_with_next_point() {
            winPoints(server, 2);
            winPoints(receiver, 3);   // 30-40

            game.receiverWinsPoint();

            assertEquals("Receiver wins", game.getScore());
        }
    }

    @Nested
    class Deuce {
        @Test
        void it_is_Deuce_when_both_players_get_to_40() {
            winPoints(server, 3);
            winPoints(receiver, 3);

            assertEquals("Deuce", game.getScore());
        }

        @Test
        void it_is_Ad_in_when_deuce_and_server_wins_point() {
            playToDeuce();
            game.serverWinsPoint();

            assertEquals("Ad in", game.getScore());
        }

        @Test
        void it_is_Ad_out_when_deuce_and_receiver_wins_point() {
            playToDeuce();
            game.receiverWinsPoint();

            assertEquals("Ad out", game.getScore());
        }

        @Test
        void server_wins_with_two_points_after_deuce() {
            playToDeuce();
            winPoints(server, 2);

            assertEquals("Server wins", game.getScore());
        }

        @Test
        void receiver_wins_with_two_points_after_deuce() {
            playToDeuce();
            winPoints(receiver, 2);

            assertEquals("Receiver wins", game.getScore());
        }

        @Test
        void it_goes_back_to_Deuce_when_Ad_in_and_receiver_wins_point() {
            playToDeuce();
            game.serverWinsPoint();    // Ad in
            game.receiverWinsPoint();  // Deuce

            assertEquals("Deuce", game.getScore());
        }

        @Test
        void it_goes_back_to_Deuce_when_Ad_out_and_server_wins_point() {
            playToDeuce();
            game.receiverWinsPoint();  // Ad out
            game.serverWinsPoint();    // Deuce

            assertEquals("Deuce", game.getScore());
        }

        @Test
        void it_goes_back_to_Deuce_with_multiple_ties() {
            playToDeuce();
            winPoints(receiver, 1); // Ad out
            winPoints(server, 2);   // Deuce, Ad in
            winPoints(receiver, 2); // Deuce, Ad out
            winPoints(server, 1);   // Deuce

            assertEquals("Deuce", game.getScore());
        }

        private void playToDeuce() {
            winPoints(receiver, 3);
            winPoints(server, 3);
        }
    }

    private static void winPoints(Procedure who, int times) {
        for (int i = 0; i < times; i++) {
            who.run();
        }
    }
}

@FunctionalInterface
interface Procedure {
    void run();

    default Procedure andThen(Procedure after){
        return () -> {
            this.run();
            after.run();
        };
    }

    default Procedure compose(Procedure before){
        return () -> {
            before.run();
            this.run();
        };
    }
}

This is AFTER I refactored:

import org.junit.jupiter.api.*;

import static org.junit.jupiter.api.Assertions.*;

@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)
class TennisGameTest {

    private TennisGame game;

    // reuseable/composable game scenarios
    private Scenario serverWinsPoint;
    private Scenario receiverWinsPoint;

    @BeforeEach
    void setUp() {
        game = new TennisGame();

        serverWinsPoint = () -> game.serverWinsPoint();
        receiverWinsPoint = () -> game.receiverWinsPoint();
    }

    @Nested
    class Basic_Scoring {

        @Test
        void it_starts_at_love_all() {
            assertEquals("Love all", game.getScore());
        }

        @Test
        void it_gives_server_score_first() {
            serverWinsPoint.play();
            assertEquals("15 - Love", game.getScore());

            serverWinsPoint.play();
            assertEquals("30 - Love", game.getScore());

            serverWinsPoint.play();
            assertEquals("40 - Love", game.getScore());
        }

        @Test
        void it_gives_receiver_score_second() {
            receiverWinsPoint.play();
            assertEquals("Love - 15", game.getScore());

            receiverWinsPoint.play();
            assertEquals("Love - 30", game.getScore());

            receiverWinsPoint.play();
            assertEquals("Love - 40", game.getScore());
        }

        @Test
        void it_uses_all_when_score_is_tied() {
            receiverWinsPoint.andThen(serverWinsPoint).play();
            assertEquals("15 all", game.getScore());

            serverWinsPoint.andThen(receiverWinsPoint).play();
            assertEquals("30 all", game.getScore());
        }

    }

    @Nested
    class Basic_Win {
        @Test
        void server_wins_with_four_points() {
            serverWinsPoint.times(4).play();

            assertEquals("Game, Server", game.getScore());
        }

        @Test
        void receiver_wins_with_four_points() {
            receiverWinsPoint.times(4).play();

            assertEquals("Game, Receiver", game.getScore());
        }

        @Test
        void at_40_30_server_wins_game_with_next_point() {
            serverWinsPoint.times(3)
                .andThen(receiverWinsPoint.times(2))
                .play();

            serverWinsPoint.play();

            assertEquals("Game, Server", game.getScore());
        }

        @Test
        void at_30_40_receiver_wins_game_with_next_point() {
            serverWinsPoint.times(2)
                .andThen(receiverWinsPoint.times(3))
                .play();

            receiverWinsPoint.play();

            assertEquals("Game, Receiver", game.getScore());
        }

    }
    @Nested
    class Deuce {
        private Scenario deuce;

        @BeforeEach
        void setUp() {
            deuce = serverWinsPoint.times(3)
                    .andThen(receiverWinsPoint.times(3));
        }

        @Test
        void it_is_Deuce_when_each_player_wins_3_points() {
            serverWinsPoint.times(3)
                .andThen(receiverWinsPoint.times(3))
                .play();

            assertEquals("Deuce", game.getScore());
        }

        @Test
        void it_is_Ad_in_when_deuce_and_server_wins_point() {
            deuce.andThen(serverWinsPoint).play();

            assertEquals("Ad in", game.getScore());
        }

        @Test
        void it_is_Ad_out_when_deuce_and_receiver_wins_point() {
            deuce.andThen(receiverWinsPoint).play();

            assertEquals("Ad out", game.getScore());
        }

        @Test
        void server_wins_with_two_points_after_deuce() {
            deuce.andThen(serverWinsPoint.times(2)).play();

            assertEquals("Game, Server", game.getScore());
        }

        @Test
        void receiver_wins_with_two_points_after_deuce() {
            deuce.andThen(receiverWinsPoint.times(2)).play();

            assertEquals("Game, Receiver", game.getScore());
        }

        @Test
        void it_goes_back_to_Deuce_when_Ad_in_and_receiver_wins_point() {
            deuce.andThen(serverWinsPoint).andThen(receiverWinsPoint).play();

            assertEquals("Deuce", game.getScore());
        }

        @Test
        void it_goes_back_to_Deuce_when_Ad_out_and_server_wins_point() {
            deuce.andThen(receiverWinsPoint).andThen(serverWinsPoint).play();

            assertEquals("Deuce", game.getScore());
        }

        @Test
        void it_goes_back_to_Deuce_with_multiple_ties() {
            deuce
                .andThen(receiverWinsPoint)           // Ad Out
                .andThen(serverWinsPoint.times(2))    // Deuce, Ad In
                .andThen(receiverWinsPoint.times(2))  // Deuce, Ad Out
                .andThen(serverWinsPoint)             // Deuce
                .play();

            assertEquals("Deuce", game.getScore());
        }
    }

    @Nested
    class Exception_Conditions {
        @Test
        void it_throws_IllegalStateException_if_point_awarded_after_player_wins() {
            serverWinsPoint.times(4).play();
            assertThrows(IllegalStateException.class, () -> game.receiverWinsPoint());
            assertThrows(IllegalStateException.class, () -> game.serverWinsPoint());
        }
    }
}

@FunctionalInterface
interface Scenario {
    void play();

    default Scenario times(int count) {
        Scenario sequence = this;
        for (int i = 1; i < count; i++) {
            sequence = sequence.andThen(this);
        }
        return sequence;
    }

    default Scenario andThen(Scenario after){
        return () -> {
            this.play();
            after.play();
        };
    }

    default Scenario compose(Scenario before){
        return () -> {
            before.play();
            this.play();
        };
    }
}

The one main concern I have with the AFTER version is that the TennisGame API is somewhat abstracted away in the Scenario lambdas. If tests are supposed to be examples for how to use the Class Under Test's API, then the AFTER version doesn't really do a good job at it. I do kind of like the way the intent of the test is expressed though.

Which version do you like? Any alternative refactoring you might suggest?

[Campbell Ritchie]

I also think the testing in the BEFORE version looks much more straightforward.

Problem: does a tennis player have a score? I know they score points, but their score depends partially o the opponent's points. If you are playing football and the opponents have 3 goals, and they score, they now have 4. {Round here, the concept of “our” team having goals doesn't apply). And when the opponents win, they do by a certain number of goals. The score doesn't immediately disappear into think are like in tennis. In tennis, if you have 30 points and score, your score is different depending on how many points the opponent has. So maybe it is better to regard the game as having a score. Just maybe.

[Junilu Lacar]

Campbell Ritchie wrote:So maybe it is better to regard the game as having a score.

Doesn't game.getScore() reflect that idea though? Not sure what brought on the question "Does a tennis player have a score?" The API is game.serverWinsPoint() which is consistent with the terms "Break point," "Game point," "Set point," and "Match point." So, yeah, I think it's quite reasonable to call the things that players win and accumulate as "points".

[Junilu Lacar]

Campbell Ritchie wrote:In tennis, if you have 30 points and score, your score is different depending on how many points the opponent has.

I don't quite get this. Can you clarify what you mean, maybe with an example?

If I've won 2 points so far in the game and I'm serving, the score will be "30 - (whatever the receiver has)".  My "30" part of the score doesn't vary depending on how well the receiver does, it's always going to be dependent on how many points I've won.

Did you mean that the "score" is a combination of the server's and receiver's states? If so, yes, of course it is. But now I'm not sure what your point (pun intended) is.

[Campbell Ritchie]

If the server scores the next point, 30-40 goes to deuce, but 30 all goes to 40-30. I am just wondering whether the players or the game should best record the score. Maybe one should record points as 8-7 and call that 'vantage in. Maybe not. I am just wondering aloud.
And if awful puns are going to set in, I shall do my best to match them.

[Junilu Lacar]

I managed to implement the functionality without an extra Player class, just having the Game class track server and receiver points.  As I mentioned in the other thread, the scheme really is that Deuce is always 3-3, Ad In is 4-3, and Ad Out is 3-4. If any player has an advantage and loses it, then their 4 gets decremented back to 3. I don't see any need to complicate it any further than that.

[Junilu Lacar]

Here's the implementation code and the note that goes with it:

    // Implementation Note:
    // Deuce always goes back to 3-3
    // Ad-in is 4-3, Ad-out is 3-4

    private boolean isDeuce() {
        return serverPoints == 3 && receiverPoints == 3;
    }

    private void receiverLosesAdvantage() {
        receiverPoints--;
    }

    private void serverLosesAdvantage() {
        serverPoints--;
    }

[Campbell Ritchie]

It would appear we were thinking on the same lines about where to record the total scores. I would never have thought of changing 'vantage X (4‑3) to 3‑3 (deuce) if Y scores the next point.

[Junilu Lacar]

Campbell Ritchie wrote:If the server scores the next point, 30-40 goes to deuce, but 30 all goes to 40-30. I am just wondering whether the players or the game should best record the score.

I think I see where our lines of thinking diverge. Again, I'm tracking points (0,1,2,3,4) internally. The Game.getScore() is responsible for translating the point combinations into "the score" -- so I have conditional statements in that method like this:

if (tied()) {
    return isDeuce() ? "Deuce" : tiedScore();
}

I take care of all special conditions first: if there's already a winner, if the game is tied, if the game is in a tieBreak situation (Ad In or Ad Out). If none of these special conditions exists, then getScore just returns the plain old "%s - %s" score.

[Junilu Lacar]

Campbell Ritchie wrote:It would appear we were thinking on the same lines about where to record the total scores. I would never have thought of changing 'vantage X (4‑3) to 3‑3 (deuce) if Y scores the next point.

I don't know if I my little story about being a ball boy and keeping score for my dad back in the day made sense, but that's exactly the experience I'm drawing from here. Scoreboard had only four disks for either side to keep track of the game score. So no matter how many times they played to deuce, you just had to keep sliding the red advantage disks back and forth to show who had the advantage.

 Serve    Receive
RWWW---||---WWWR   (Love all)

RWW---W||---WWWR   (15 - Love)

R---WWW||WWW---R   (Deuce)

---RWWW||WWW---R   (Ad In)

R---WWW||WWWR---   (Ad Out)


You can't make this stuff up. Here's a link to an old blurry picture: https://images.app.goo.gl/c71MahzowtDJerHJ9 of the tennis courts where my dad used to play.

If you click on the dark blurry one a couple of times, you might be able to make out the white and red disks that I'm talking about just behind the player's head. There were two rows of disks, the top row to keep the game score, and the bottom row of disks to keep a tally of games won in the set.

Here's one that's clearer: https://thetalesoftheeasyrider.files.wordpress.com/2011/04/tennisgraduation.jpg -- those scoring stands look newer than the ones we had in the 1970s/80s but the disks are still there. What a blast from the past.

[Piet Souris]

As refactoring both look fine indeed!

I do have some critics though. By tests like 'void it_gives_server_score_first()' you put a constraint on how the output should be. In the specific topic I showed a Wimbledon scoreboard, that gives the score independant of who is serving.

Something similar holds for the tests that uses terms like "all" and "deuce" and "Advantage in". Now, I know these are terms used when saying out the score, like an umpire does, but it is not necessary to show these terms on screen; a simple '40-40' would do. Yet the tests require such output.

Are there requirements that lead to theses tests, so I wonder?

I really like your 'Scenario' (Procedure is also a fine term). You can shorten it a little, by letting it extend Runnable. You can then let it execute by an Executor (though not relevant here). I made this text:

interface Junilu extends Runnable {
    default Junilu andThen(Runnable r) {
        return () -> {this.run(); r.run();};
    }
    
    default Junilu times(int n) {
        return Stream.generate(() -> this).limit(n).reduce(() -> {}, (a, b) -> a.andThen(b));
    }

    // compose left out
}

[Junilu Lacar]

Piet Souris wrote:Are there requirements that lead to theses tests, so I wonder?

The OP's requirements in the other thread didn't call those out but yes, I did add them as "requirements." Given that the UI simply had one option to display the score, the API only had one method to get the "combined" score. I know from experience that the server's score is always mentioned first so if the server wins the first point, the score will be "15 - Love" and if the receiver wins the first point it will be "Love - 15." Thus the tests capture the need for a specific order in the combined score. If the view was different, like the scoreboard you suggested, I would imagine the API design would be slightly different as well and the tests would reflect that.

This could lead to an interesting question though. Should you allow the view to dictate the API? If the API were to only have getServerScore() and getReceiverScore() then the "server's score should be mentioned first" logic probably won't be in the Model but in the View. Would this be appropriate? I think it could be argued both ways but I'd probably end up putting the logic in the View if those were the requirements. Might be an interesting next exercise.