diff --git a/Sources/Model/PlayerManager.cs b/Sources/Model/PlayerManager.cs index b14c07f..a23910f 100644 --- a/Sources/Model/PlayerManager.cs +++ b/Sources/Model/PlayerManager.cs @@ -1,10 +1,6 @@ using System; using System.Collections.Generic; -using System.Collections.ObjectModel; -using System.Diagnostics; using System.Linq; -using System.Text; -using System.Threading.Tasks; namespace Model { @@ -13,7 +9,7 @@ namespace Model /// /// a hashset of the players that this manager is in charge of ///
- /// the hash is based on the player's unique property (name) + /// each player is unique (set), and the hash is based on the player's values (name) ///
private readonly HashSet players; @@ -31,19 +27,20 @@ namespace Model if (toAdd != null) { players.Add(toAdd); + return toAdd; } - return toAdd; + throw new ArgumentNullException(nameof(toAdd), "param should not be null"); } /// - /// may never get implemented in the model, go through GetOneByName() in the meantime + /// might never get implemented in the model, go through GetOneByName() in the meantime /// /// /// /// public Player GetOneById(int id) { - throw new NotImplementedException("may never get implemented\ngo through GetOneByName() in the meantime"); + throw new NotImplementedException("might never get implemented\ngo through GetOneByName() in the meantime"); } /// @@ -52,22 +49,23 @@ namespace Model /// that copy does not belong to this manager's players, so it should not be modified /// /// a player's unique name - /// player with said name + /// player with said name, or null if no such player was found public Player GetOneByName(string name) { if (!String.IsNullOrWhiteSpace(name)) { - Player result = players.FirstOrDefault(p => p.Name.ToUpper().Equals(name.Trim().ToUpper())); - if (result == null) return result; // will return null by default if no such player exists - return new(result); // THIS IS A COPY (using a copy constructor) + Player wanted = new(name); + Player result = players.FirstOrDefault(p => p.Equals(wanted)); + return result is null ? null : new Player(result); // THIS IS A COPY (using a copy constructor) } - return null; // we also want ot return null if no name was provided + throw new ArgumentException("param should not be null or blank", nameof(name)); } /// /// get a READ ONLY enumerable of all players belonging to this manager + /// so that the only way to modify the collection of players is to use this class's methods /// - /// a readonly list of all this manager's players + /// a readonly enumerable of all this manager's players public IEnumerable GetAll() => players.AsEnumerable(); /// @@ -78,6 +76,15 @@ namespace Model /// updated player public Player Update(Player before, Player after) { + Player[] args = { before, after }; + + foreach (Player player in args) + { + if (player is null) + { + throw new ArgumentNullException(nameof(player), "param should not be null"); + } + } Remove(before); return Add(after); } @@ -88,12 +95,12 @@ namespace Model /// player to be removed public void Remove(Player toRemove) { - // delegating, making sure we find it even if different case etc. - if (toRemove != null) + if (toRemove is null) { - Player realToRemove = GetOneByName(toRemove.Name); - players.Remove(realToRemove); + throw new ArgumentNullException(nameof(toRemove), "param should not be null"); } + // the built-in HashSet.Remove() method will use our redefined Equals(), using Name only + players.Remove(toRemove); } } } diff --git a/Sources/Tests/Model_UTs/PlayerManagerTest.cs b/Sources/Tests/Model_UTs/PlayerManagerTest.cs index b4ac083..f793455 100644 --- a/Sources/Tests/Model_UTs/PlayerManagerTest.cs +++ b/Sources/Tests/Model_UTs/PlayerManagerTest.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; using System.Collections.ObjectModel; using System.Linq; +using System.Xml.Linq; using Xunit; namespace Tests.Model_UTs @@ -46,20 +47,20 @@ namespace Tests.Model_UTs } [Fact] - public void TestAddIfNullThenDontAddAndReturnNull() + public void TestAddIfNullThrowsException() { // Arrange PlayerManager playerManager = new(); Player expected; - Player actual; // Act expected = null; - actual = playerManager.Add(expected);// Add() returns the added element if succesful + void action() => playerManager.Add(expected);// Add() returns the added element if succesful // Assert - Assert.Equal(expected, actual); + Assert.Null(expected); Assert.DoesNotContain(expected, playerManager.GetAll()); + Assert.Throws(action); } [Fact] @@ -79,7 +80,7 @@ namespace Tests.Model_UTs [InlineData("")] [InlineData(null)] [InlineData(" ")] - public void TestGetOneByNameIfInvalidThenReturnNull(string name) + public void TestGetOneByNameIfInvalidThrowsException(string name) { // Arrange PlayerManager playerManager = new(); @@ -87,10 +88,10 @@ namespace Tests.Model_UTs playerManager.Add(player); // Act - Player result = playerManager.GetOneByName(name); + void action() => playerManager.GetOneByName(name); // Assert - Assert.Null(result); + Assert.Throws(action); } [Fact] @@ -143,21 +144,17 @@ namespace Tests.Model_UTs } [Fact] - public void TestRemoveFailsSilentlyIfGivenNull() + public void TestRemoveThrowsExceptionIfGivenNull() { // Arrange PlayerManager playerManager = new(); - Player player = new("Dylan"); - playerManager.Add(player); - Player notPlayer = null; - IEnumerable expected = new Collection { player }; + playerManager.Add(new Player("Dylan")); // Act - playerManager.Remove(notPlayer); - IEnumerable actual = playerManager.GetAll(); + void action() => playerManager.Remove(null); // Assert - Assert.Equal(actual, expected); + Assert.Throws(action); } [Fact] @@ -215,8 +212,92 @@ namespace Tests.Model_UTs // Assert Assert.Contains(oldPlayer, playerManager.GetAll()); Assert.Contains(newPlayer, playerManager.GetAll()); - Assert.Equal(n2.Trim(), playerManager.GetAll().First().Name); - // uses Equals(), which is made to be case-insensitive + Assert.Equal(oldPlayer, newPlayer); + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + [InlineData(null)] + public void TestUpdateDoesNotGoWithValidBeforeAndInvalidAfter(string badName) + { + // Arrange + PlayerManager playerManager = new(); + Player oldPlayer = new("Ni!"); + playerManager.Add(oldPlayer); + int size1 = playerManager.GetAll().Count(); + + // Act + Assert.Contains(oldPlayer, playerManager.GetAll()); + void action() => playerManager.Update(oldPlayer, new Player(badName));// this is really testing the Player class... + int size2 = playerManager.GetAll().Count(); + + // Assert + Assert.Throws(action); // thrown by Player constructor + Assert.Contains(oldPlayer, playerManager.GetAll()); // still there + Assert.True(size1 == size2); + } + + [Fact] + public void TestUpdateDoesNotGoWithValidBeforeAndNullAfter() + { + // Arrange + PlayerManager playerManager = new(); + Player oldPlayer = new("Ni!"); + playerManager.Add(oldPlayer); + int size1 = playerManager.GetAll().Count(); + + // Act + Assert.Contains(oldPlayer, playerManager.GetAll()); + void action() => playerManager.Update(oldPlayer, null); + int size2 = playerManager.GetAll().Count(); + + // Assert + Assert.Throws(action); // thrown by Update() + Assert.Contains(oldPlayer, playerManager.GetAll()); // still there + Assert.True(size1 == size2); + } + + [Fact] + public void TestUpdateDoesNotGoWithValidAfterAndNullBefore() + { + // Arrange + PlayerManager playerManager = new(); + Player newPlayer = new("Kevin"); + Player oldPlayer = new("Ursula"); + playerManager.Add(oldPlayer); + int size1 = playerManager.GetAll().Count(); + + // Act + void action() => playerManager.Update(null, newPlayer); + int size2 = playerManager.GetAll().Count(); + + // Assert + Assert.Throws(action); // thrown by Update() + Assert.Contains(oldPlayer, playerManager.GetAll()); // still there + Assert.True(size1 == size2); + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + [InlineData(null)] + public void TestUpdateDoesNotGoWithValidAfterAndInvalidBefore(string name) + { + // Arrange + PlayerManager playerManager = new(); + Player oldPlayer = new("Ursula"); + playerManager.Add(oldPlayer); + int size1 = playerManager.GetAll().Count(); + + // Act + void action() => playerManager.Update(new Player(name), new Player("Vicky")); + int size2 = playerManager.GetAll().Count(); + + // Assert + Assert.Throws(action); // thrown by Player constructor + Assert.Contains(oldPlayer, playerManager.GetAll()); // still there + Assert.True(size1 == size2); } } }