Football game using factory and command patterns

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:

-
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'"

-
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"

-
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.

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

-
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

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.

StackExchange Code Review Q#124999, answer score: 7