diff --git a/Sources/Model/Player.cs b/Sources/Model/Player.cs index a4a949e..32cb8b4 100644 --- a/Sources/Model/Player.cs +++ b/Sources/Model/Player.cs @@ -1,41 +1,34 @@ using System; -using System.Xml.Linq; namespace Model { + /// + /// A player for the purpose of a game of dice, consists of a name + ///
+ /// Two players are equal for Equals() and GetHashCode() if they have the same name, once trimmed and case-insensitive + ///
public sealed class Player : IEquatable { - public string Name + /// + /// a player's unique username + /// + public string Name { get; private set; } + public Player(string name) { - get - { - return name; - } - internal set + if (!String.IsNullOrWhiteSpace(name)) { - if (!String.IsNullOrWhiteSpace(value)) - { - name = value.Trim(); - } - else throw new ArgumentException("player name may never be empty or null"); + Name = name.Trim(); } + else throw new ArgumentException("param should not be null or blank", nameof(name)); } - private string name; - - public Player(string name) - { - Name = name; - } - + /// + /// this is a copy contructor + /// + /// a player object to be copied public Player(Player player) - { - if (player != null) - { - Name = player.name; - } - else throw new ArgumentException("you may not make a copy of a null player"); - } + : this(player?.Name) // passes the player's name if player exists, else null + { } public override string ToString() { @@ -44,7 +37,7 @@ namespace Model public bool Equals(Player other) { - return Name.ToUpper() == other.Name.ToUpper(); + return Name.ToUpper() == other.Name.ToUpper(); // equality is case insensitive } public override bool Equals(Object obj) @@ -53,12 +46,12 @@ namespace Model { return false; } - return Equals(obj as Player); + return Equals(obj as Player); // casting, to send it to the above Equals() method } public override int GetHashCode() { - return Name.ToUpper().GetHashCode(); + return Name.ToUpper().GetHashCode(); // hash is case insensitive } } } diff --git a/Sources/Model/PlayerManager.cs b/Sources/Model/PlayerManager.cs index b14c07f..806ef4b 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,17 @@ 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(after), "param should not be null"); + // could also be because of before, but one param had to be chosen as an example + // and putting "player" there was raising a major code smell + } + } Remove(before); return Add(after); } @@ -88,12 +97,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 f3f87dc..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,23 +47,22 @@ 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); } - // TODO update if we do implement it [Fact] public void TestGetOneByIdThrowsNotImplemented() { @@ -80,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(); @@ -88,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] @@ -144,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] @@ -216,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); } } }