diff --git a/README.md b/README.md index b55ce599349d912017a852dc24c6a653ee10fca1..4c12488b090eaad460aba0721c0c0c766974e4fb 100644 --- a/README.md +++ b/README.md @@ -34,3 +34,23 @@ Mexico 0 - Canada 5 Argentina 3 - Australia 1 Germany 2 - France 2 ``` + +###Some notes on edge cases +In an exercise where simplicity is requested and edge cases as well, it's virtually impossible to match expectations. This makes this exercise great. + +My approach for not dealing with nulls, but still dealing with nulls, in a simple project like this, was this: +* There is no reference to nulls in the method javadocs, so they are not expected and therefore the behaviour is undetermined +* In an application using this library, as it was before the commit where these notes got added, the cases where nulls would be passed are extreme and arguably not something that should be dealt with at this level in an app +* Although I didn't add a test for it, I made sure that an NPE would be the exception being thrown +* Adding it properly to where it's needed is time-consuming. I have limited time to do these challenges so the compromise was to make sure the correct exceptions were thrown with the code I delivered + +I tend to think and code for edge cases that stem from user error, not programmer error. NPEs, as any Runtime Exception, are to be avoided and functionality made to fail gracefully. They most often result from programmer error or hardware faults so... got to pick battles.. + +####An edge case I got wrong +Trying to finish a match that is no longer held by the library, or never was, seems like something a user would do by mistake and is definitely something that one such library should trigger an exception for, in general. +However, I did everything but add the javadoc throws tag for it. :) Added it now. + +Trying to start a match with teams that are already in a game is another example. Or the same team as away and home. +However, in these I added everything but the unit tests for them. + +Adding isInOngoingMatch(ITeam) and isOngoingMatch(IMatch) would also be nice helper methods to have. But..simple library. diff --git a/src/main/java/org/example/scoreboard/api/IScoreboard.java b/src/main/java/org/example/scoreboard/api/IScoreboard.java index b92e9ef5a14cc93b7b24ca00cbdceba6009c782e..8024fba7e69034d2b7f5fec1808af7e13ecd2067 100644 --- a/src/main/java/org/example/scoreboard/api/IScoreboard.java +++ b/src/main/java/org/example/scoreboard/api/IScoreboard.java @@ -2,6 +2,7 @@ package org.example.scoreboard.api; import java.security.InvalidParameterException; import java.util.ArrayList; +import java.util.NoSuchElementException; public interface IScoreboard { @@ -11,7 +12,8 @@ public interface IScoreboard { * * @param homeTeam the home team * @param awayTeam the away team - * @throws InvalidParameterException the team is already in a match + * @throws InvalidParameterException team is already in a match or the same team is passed as home and away + * @throws NullPointerException one, or both, of the passed teams is null * @return the match started */ IMatch startMatch(ITeam homeTeam, ITeam awayTeam); @@ -21,6 +23,8 @@ public interface IScoreboard { * It will remove a match from the scoreboard. * * @param match the match to finish + * @throws NoSuchElementException match is not ongoing + * @throws NullPointerException passed match is null * @return the finished match */ IMatch finishMatch(IMatch match); @@ -32,6 +36,7 @@ public interface IScoreboard { * @param match match to update * @param homeTeamScore home team score * @param awayTeamScore away team score + * @throws NullPointerException passed match is null or sores are null or goals in scores are null * @return updated match */ IMatch updateScore(IMatch match, IScore homeTeamScore, IScore awayTeamScore); diff --git a/src/main/java/org/example/scoreboard/generic/AbstractScoreboard.java b/src/main/java/org/example/scoreboard/generic/AbstractScoreboard.java index 0b40a64af4a9997c66084abc6aca6416bf0f76de..93c447c0dd9cae8a273fb7fda7bbc8a9f3de11e3 100644 --- a/src/main/java/org/example/scoreboard/generic/AbstractScoreboard.java +++ b/src/main/java/org/example/scoreboard/generic/AbstractScoreboard.java @@ -29,6 +29,10 @@ public class AbstractScoreboard implements IScoreboard { @Override public IMatch startMatch(ITeam homeTeam, ITeam awayTeam) { + if (homeTeam == null || awayTeam == null) { + throw new NullPointerException(); + } + if(teamsOnTheBoard.putIfAbsent(homeTeam.getId(), homeTeam) != null) { throw new InvalidParameterException("Home team already on the board"); } @@ -45,6 +49,9 @@ public class AbstractScoreboard implements IScoreboard { @Override public IMatch finishMatch(IMatch match) { + if (match == null) { + throw new NullPointerException(); + } if (!ongoingMatches.containsKey(match.getId())) { throw new NoSuchElementException(); } @@ -55,6 +62,10 @@ public class AbstractScoreboard implements IScoreboard { @Override public IMatch updateScore(IMatch match, IScore homeTeamScore, IScore awayTeamScore) { + if (match == null || homeTeamScore == null || awayTeamScore == null || homeTeamScore.getGoals() == null + || awayTeamScore.getGoals() == null) { + throw new NullPointerException(); + } if (!ongoingMatches.containsKey(match.getId())) { throw new NoSuchElementException(); } diff --git a/src/test/java/org/example/scoreboard/reference/TestScoreboard.java b/src/test/java/org/example/scoreboard/reference/TestScoreboard.java index 5aff9423e10a2c647c1fdfceb1736b239eec4cc4..a613eb62544f0bd844df2836b87ee5c4027c636d 100644 --- a/src/test/java/org/example/scoreboard/reference/TestScoreboard.java +++ b/src/test/java/org/example/scoreboard/reference/TestScoreboard.java @@ -6,6 +6,7 @@ import org.junit.After; import org.junit.Before; import org.junit.Test; +import java.security.InvalidParameterException; import java.util.*; import java.util.stream.Stream; @@ -65,6 +66,19 @@ public class TestScoreboard { assertNotNull(scoreboard.startMatch(Mexico, Canada)); } + // should throw InvalidParameterException if a match is started with a team already on a match + @Test + public void shouldThrowInvalidParameterException_whenMatchContainsTeamInOngoingMatches() { + scoreboard.startMatch(Mexico, Canada); + assertThrows(InvalidParameterException.class, () -> scoreboard.startMatch(Mexico, Canada)); + } + + // should throw InvalidParameterException if a match is between 1 team, home is the same as away team + @Test + public void shouldThrowInvalidParameterException_whenMatchHasHomeAndAwayAsSameTeam() { + assertThrows(InvalidParameterException.class, () -> scoreboard.startMatch(Mexico, Mexico)); + } + // should finish a game when the game has been started before @Test public void shouldFinishAGame_whenGameHasBeenStarted() { @@ -147,4 +161,42 @@ public class TestScoreboard { Object[] gameSummary = scoreboard.getGameSummary().toArray(); assertArrayEquals(expectedResult, gameSummary); } + + // should throw NullPointerException when teams are null in start of a match + @Test + public void shouldThrowNullPointerException_whenTeamsAreNullInStartMatch() { + assertThrows(NullPointerException.class, () -> scoreboard.startMatch(null, Canada)); + assertThrows(NullPointerException.class, () -> scoreboard.startMatch(Mexico, null)); + assertThrows(NullPointerException.class, () -> scoreboard.startMatch(null, null)); + } + + // should throw NullPointerException when the team is null in the match finish + @Test + public void shouldThrowNullPointerException_whenMatchIsNullInFinishMatch() { + assertThrows(NullPointerException.class, () -> scoreboard.finishMatch(null)); + } + + // should throw NullPointerException when the match is null in the match update + @Test + public void shouldThrowNullPointerException_whenMatchIsNullInUpdateMatch() { + assertThrows(NullPointerException.class, () -> scoreboard.updateScore(null, new Score(0), new Score(5))); + } + + // should throw NullPointerException when the scores are null in the match update + @Test + public void shouldThrowNullPointerException_whenScoresAreNullInUpdateMatch() { + IMatch match = scoreboard.startMatch(Mexico, Canada); + assertThrows(NullPointerException.class, () -> scoreboard.updateScore(match, null, null)); + assertThrows(NullPointerException.class, () -> scoreboard.updateScore(match, null, new Score(5))); + assertThrows(NullPointerException.class, () -> scoreboard.updateScore(match, new Score(0), null)); + } + + // should throw NullPointerException when the goals in scores are null in the match update + @Test + public void shouldThrowNullPointerException_whenGoalsInScoresAreNullInUpdateMatch() { + IMatch match = scoreboard.startMatch(Mexico, Canada); + assertThrows(NullPointerException.class, () -> scoreboard.updateScore(match, new Score(null), new Score(5))); + assertThrows(NullPointerException.class, () -> scoreboard.updateScore(match, new Score(0), new Score(null))); + assertThrows(NullPointerException.class, () -> scoreboard.updateScore(match, new Score(null), new Score(null))); + } }