Football game using factory and command patterns

Question

I'm really confused about the interview I did with a firm. They told my code was not expert, but almost intermediate level java.

Requirements

Football Scoring Dashboard

Develop an application that prints out a scoring dashboard as text during a football match. The football scoring dashboard would have output the following at the 80th minute in the 1966 Football world cup final between England and West Germany: "England 2 (Hurst 18' Peters 78') vs. West Germany 1 (Haller 12')". The application’s required inputs are singular entries following the following flow:

1. The Football Scoring Dashboard needs to know when a game starts through being supplied a string of this format: "Start: '' vs. ''".

   Example: "Start: 'England' vs. 'West Germany'"

2. After the start command has been given, acceptable inputs to tell the Dashboard when goals are scored follow the following structure: " '' ".

   Example: "11 'West Germany' Haller"

3. The tool should be able to compute the 'print' command at any time during the course of a game to print the aggregated scoring statistics of the match.

   Example: If tool is given the 'print' command, it should output the following: "England 0 vs. West Germany 1 (Haller 12')" if that is the only goal that has been scored at that point.

4. The Dashboard knows a game has ended through the 'End' command.

5. The tool should cater for the following error conditions:

   If the Football Scoring Dashboard is given any commands while a game is not in progress it should report 'No game currently in progress'.

   If a game is in progress and it is not able to understand the given command it should return: 'input error - please type 'print' for game details'.

   If a game is not in progress and it is not able to understand the given command, it should return: 'input error - please start a game through typing 'Start: '' vs. '''.

Basically, the problem was to create a scoreboard to print the game status depending on 4 entries (inputs). Around 4-5 hours max.

My solution

Game main class.

package game;

import java.util.Map;
import java.util.TreeMap;
import java.util.logging.Logger;

/**
 * @author FNERON
 * Implements the game for a football.
 * 
 */
public class Football implements IGame {

    public static enum State {
        START, PLAYING ,END
    }

    private final static Logger LOGGER = Logger.getLogger(Football.class.getName());

    private String awayTeam;
    private String homeTeam;

    private int awayScore;
    private int homeScore;

    private Map<Integer, String> goalAwayTeam;
    private Map<Integer, String> goalHomeTeam;

    private State state;

    /**
     * Constructor
     */
    public Football(){
        reset();
    }

    /**
     * Start a new game
     * @param String awayTeam, String homeTeam
     * @return String message
     */
    @Override
    public String start(String awayTeam, String homeTeam) {
        reset();

        state = State.START;

        this.awayTeam = awayTeam;
        this.homeTeam = homeTeam;

        return String.format("%s %d vs. %s %d\n", this.awayTeam, awayScore, this.homeTeam, homeScore);

    }

    /**
     * Print the current game info.
     * @return String message
     */
    @Override
    public String print() {
        if (state == State.END) {
            return "No game currently in progress";
        }

        return printInfo();
    }

    /**
     * Adds a goal to the game.
     * @param int minutes, String team, String player
     * @return String message
     */
    @Override
    public String score(int minutes, String team, String player) {
        if (state == State.END) {
            return "No game currently in progress";
        }

        state = State.PLAYING;

        if(this.awayTeam.equals(team)) {
            awayScore++;
            goalAwayTeam.put(minutes, player);
        }

        if(this.homeTeam.equals(team)) {
            homeScore++;
            goalHomeTeam.put(minutes, player);
        }

        return "Goal!!!\n" + printInfo();
    }

    /**
     * Finish the game.
     * @return String message
     */
    @Override
    public String end() {
        if(state == State.END) {
            return "No game currently in progress";
        }

        String info = printInfo();

        state = State.END;
        reset();

        return "The game has ended!\n" + info;  
    }

    /**
     * Will return the current info on the football game as a String.
     * TODO: Use the StringBuffer for optimization.
     * @return String info
     */
    public String printInfo(){

        String string = awayTeam + " " + awayScore + " ";

        if (!goalAwayTeam.isEmpty()){
            string += "(";
            for (Map.Entry<Integer, String> entry : goalAwayTeam.entrySet()) {
                string += entry.getValue() + " " + entry.getKey() + "' ";
            }
            string = string.substring(0, string.length() -1);
            string += ")";
        }

        string += " vs. " + homeTeam + " " + homeScore + " ";

        if (!goalHomeTeam.isEmpty()){
            string += "(";
            for (Map.Entry<Integer, String> entry : goalHomeTeam.entrySet()) {
                string += entry.getValue() + " " + entry.getKey() + "' ";
            }
            string = string.substring(0, string.length() -1);
            string += ")";
        }

        return string;
    }

    /**
     * Handles the invalid input on state.
     * @return String message
     */
    @Override
    public String invalid() {
        if((state == State.START || state == State.PLAYING )) {
            return " 'input error - please type 'print' for game details'.";
        }
        else
            return " 'input error - please start a game through typing 'Start:'<Name of Home Team>' vs. '<Name of Away Team>''.";
    }

    /**
     * Reset the game to a new one.
     */
    public void reset() {
        awayTeam = "";
        homeTeam = "";
        awayScore = 0;
        homeScore = 0;
        goalAwayTeam = new TreeMap<Integer, String>();
        goalHomeTeam = new TreeMap<Integer, String>();
        state = State.END;
    }

    public State getState() {
        return state;
    }
}

Parser class who handle string inputs and create the respective command.

package parser;

import java.util.regex.Matcher;
import java.util.regex.Pattern;

import command.ICommand;
import command.InvalidCommand;
import command.PrintCommand;
import command.ScoreCommand;
import command.StartCommand;
import command.StopCommand;
import game.IGame;

/**
 * @author FNERON
 * Handles parsing from string inputs. Acts as a model.
 * Command and creation factory.
 */
public final class GameParser {

    private String awayTeam;
    private String homeTeam;

    private String team;
    private int minutes;
    private String player;

    private IGame game;

    /**
     * Constructor
     * @param IGame game
     */
    public GameParser(IGame game) {
        this.game = game;
    }

    /**
     * Depending inputs handle the creation of commands.
     * @param String string
     * @return ICommand command
     */
    public ICommand parse(String string) {
        if(Pattern.matches("^Start: '(.+)' vs. '(.+)'$", string)) {
            Pattern r = Pattern.compile("Start: '(.+)' vs. '(.+)'$"); 

            Matcher m = r.matcher(string);

            while (m.find()) {
                awayTeam = m.group(1);
                homeTeam = m.group(2);
            }

            return new StartCommand(game, this);
        }
        else if(Pattern.matches("^(\\d+) '(.+)' (.+)$", string)) {
            Pattern pattern = Pattern.compile("(\\d+) '(.+)' (.+)");
            Matcher matcher = pattern.matcher(string);

            while (matcher.find()) {
                minutes = Integer.parseInt(matcher.group(1));
                team = matcher.group(2);
                player = matcher.group(3);
            }

            return new ScoreCommand(game, this);
        }
        else if(string.equals("print")) {
            return new PrintCommand(game);
        }
        else if(string.equals("end")) {
            return new StopCommand(game);
        }
        else
            return new InvalidCommand(game);
    }

    public String getAwayTeam() {
        return awayTeam;
    }

    public String getHomeTeam() {
        return homeTeam;
    }

    public String getTeam() {
        return team;
    }

    public int getMinutes() {
        return minutes;
    }

    public String getPlayer() {
        return player;
    }
}

Main class.

package Application;

import Swing.Scoreboard;
import game.Football;
import parser.GameParser;


//TODO Can we handle multiple game at once ?
//TODO Do we have to take for granted that it's a score board only for football?
//TODO Add a timer to end game after 80 minutes. Not specified in problems. (Thread)
//TODO Ask more question about the game definitions (level of abstractions)
//TODO Ask for more corner case and refined current corner case.

/**
 * Develop an application that prints out a scoring dashboard as text during a football match.
 * @author FNERON
 * Possible inputs
 * 1)   Start: '<Name of Home Team>' vs. '<Name of AwayTeam>'
 * 2)   <minute> '<Team>' <name of scorer>
 * 3)   print
 * 4)   end
 */
public class ScoreboardApplication {

    public static void main(String[] args) {

        Football footBall = new Football();
        GameParser gameParser = new GameParser(footBall);

        Scoreboard window = new Scoreboard(gameParser);
        window.setVisible(true);
    }
}

My code basically implemented two design patterns (factory and command pattern). It is also tightly coupled to respect the open/closed principle. By consequent, we have dependency injection on various section of my code (because of factory pattern). Plus, I implemented quick tests with mockito. All this in around 4 hours tops. The complete code is on GitHub.

I'm wondering what can I do to make my code more "acceptable" or "expert" to that firm. I was very proud of my code when I submitted it and to receive such a un-constructive feedback throw me off.

Answer 1

Requirements?

It's hard to tell why you got the feedback you got without knowing what requirements you were given. But my first impression is that there's too much focus on patterns and too little on a correct implementation and predictable behavior.

What I mean is: if'm hiring someone I'm not looking at how many design patterns they can use. I'm looking for someone that can analyse a given set of requirements and build correct, maintainable programs that fulfill their intended purpose. Patterns are just one tool among many that can be used to achieve that.

Note: Java isn't my forte, so I won't comment on Java-specific issues.

Parser issues

The parser implementation looks troublesome. It maintains state that belongs to the commands that it creates. The ScoreCommand should have a minute, team and player property and the StartCommand should contain the names of both teams.

The current implementation only works as expected if each command is executed before the next command is parsed. Even if that behavior was documented clearly it would still be very easy to misuse. Say, a co-worker is tasked with parsing game log files that contain multiple commands. The natural thing to do would be to parse all file content before passing those commands on to code that actually does something with them.

By giving commands a reference to the parser that created them you're also making them less flexible, because they now have to be created by a parser. What if I need to generate random commands, say, for testing? What if they're serialized and sent across a network? What if there are multiple input formats and thus possibly multiple parser classes?

Also, parseCommand is a bit more descriptive than just parse.

Invalid input

Why doesn't the parsing code throw an exception on invalid input? That allows you to handle it immediately, without having to create a useless command that calls an equally useless (and, in the context of a game, confusingly named) method.

Mixed business and UI logic

The Game class performs 'business logic', but also returns strings that are shown to the user. That's mixing UI and business logic. It's better to keep those separate. Business logic may need to run in different (non-UI) environments. A UI may need to be localized, or data may need to be visualized in a different way (graphs, animations).

Game class

Technically, as soon as you start a game the players are playing, so the 'START' state doesn't really make sense. A game is either active or not, and if it's not, it has either ended or has not been started yet.

Why doesn't Game allow outside code to get the name of its teams and an overview of goals? Doing so would enable UI code to display game information (in a way that it chooses). It would also allow your test code to actually check that the various methods did what they're meant to do.

Issueing a start command resets any previous state. Perhaps it's better to create a new Game instance instead, so that each Game object actually represents the statistics of a single a game. But perhaps not - it depends on the requirements and how this program will likely be used (and thus in which directions the requirements are likely to evolve).

Tests

In your parser tests, test input is located in a single @Before method. Putting that input in the actual test methods would make them more readable and maintainable (you don't need to scroll up all the time to see what input you're actually testing).

I would also expect to see a few more edge-cases, such as malformed commands (common misspellings, missing some required characters, containing extra whitespace or other characters around required parts, uppercase or mixed case instead of lowercase, and so on).

Further thoughts

With the given requirements I don't see a compelling reason to parse commands into command objects. Just parse and execute. If, later, they want undoable commands, or a history of commands, or something like that, you can always refactor your code. It might be useful later, but not now. 'YAGNI' is what they say.

In any case, the scope that those commands operate on (a Game object) is too limited, given that some commands should be able to start and end games.

Inside Game, I'd probably add a Team and Goal class - a team has a name and a list of goals (the length of that list is its score - no need for an extra field), and each goal has a time and player name. That adds a little more structure and removes some duplicated fields from Game.

Note that using a Map to keep track of scores would fail in the highly improbable event of having one team make two goals in the same minute. Also note that the order of items in a map is not necessarily guaranteed. Since the use-case here is to print all goals, and probably in chronological order, it's not a very suitable data structure here.

--

I hope you'll find this helpful. Feel free to comment if you disagree or have questions.

Answer 2

Not a full review, just some initial thoughts:

Returning Strings

Almost all of your method return strings, which makes a class hard to use and extremely hard to test.

You can easily see this looking at the tests you did implement. What if the message changes? Now all your tests are broken, even though the logic didn't change at all. [and just as an aside: testing a string return value in these cases doesn't really test the correctness of the methods]

In most cases, returning nothing would be the better choice. For example, why should start be responsible to format and return a score string? It should only start a game. score should also only add a goal, it shouldn't need to print or return anything.

Misc

• there are some duplicate calls. For example, reset is called when beginning a game and when ending it; state = State.END is set in end, and again in reset, which is called by end.
• print: doesn't actually print anything, but returns a string. toString might be a better fit. But it's not really clear why the method is needed, and what the difference to printInfo is.
• invalid: what does this mean? It's quite unclear when I am supposed to call it.
• score: score what? getScore? setScore? not really. addGoal seems to be a better name.

Answer 3

This is some initial feedback, based on your unit tests. If you're going to submit unit tests as part of an interview application, treat them as first class citizens. My personal opinion is that it would be better not to submit unit tests for the work you've done, than to submit unit tests that are wrong or incomplete (obviously opinions on that might differ).

FootBallTest

The first thing I noticed when I looked at your tests was that in all of your assertEquals calls you're passing your expected and actual values the wrong way round. This is a fairly common error when people start unit testing because it's common for people to think 'actual value should be expected value', however the assertEquals methods expect it the other way round. Whilst they still fail the test the same way, the output is going to be wrong. Not getting this the right way round red flags your testing because it suggests you've probably never seen the tests fail and read the errors. It also calls into question whether or not you have similar attention issues with other APIs. It may seem unfair, but that's the initial impression it gives.

Your test naming isn't great. The test name should describe what it is you're attempting to test. For example, testPrintGame, without looking at the code of the test, it's really unclear what this test is supposed to do. Does it call printGame and then what, what's it expecting? A better name might be something like testDuringGamePrintShouldReturnGamePrintInformation. This appears to be what the test is actually doing.

Don't call tests from tests. It's ok to reuse code in your test classes, but don't use it directly at the test level, do it by putting the code into well named methods. Looking at testPrintInfo, the first thing you do is call testStartGame then testScoreGame. This means that your testPrintInfo method can fail if the text returned from start or score is wrong, in addition to the actual test case you care about, calling printInfo. The test could have been written so that they called the game object directly to start and score and then validated the printInfo output. This makes the test more likely to only fail when the code it cares about is actually broken.

GameParserTest

Your GameParser is being used to construct ICommand objects. From the brief look at them, they all take in an IGame reference to the game associated with the parser. At the moment, your tests validate that when parse is called the expected command type is constructed. A better approach would have been something like this (this is pseudo code since I haven't used mockito):

@Test
public void testParserConstructsInvalidCommandOnInvalidInput() {
    // Setup game mock to return known response when `invalid` is called
    when(game.invalid()).thenReturn(expectedExecuteResponse);

    // Call parse and then execute the returned command object to
    // get actual response
    ICommand tested = gameParser.parse(invalidParse);
    String actualExecuteResponse = tested.execute();

    // validate that response from executing command is the same one that
    // the game mock was setup to return
    Assert.assertEquals(expectedExecuteResponse, actualExecuteResponse);
}

This avoids needing to know the type of command object that is constructed (which is incidental) and validates that the command constructed by the parser is actually constructed with a command that does what it's supposed to (invoke the correct method on the IGame object and return the response). This also fills a hole that currently exists in your unit tests.

The above approach to testing the GameParser does link it with the ICommand object construction and treats them in as a single unit. I'm suggesting this because I think the two are intrinsically linked, however other people believe in a test class per implementation class approach, so it's important to decide for yourself which approach you believe in so that you are able to defend any decisions if the code gets you to an interview.