From fd00107e5f5501a56244f0a8bf201f985f13a1ba Mon Sep 17 00:00:00 2001 From: Alexis DRAI Date: Mon, 19 Sep 2022 09:00:00 +0200 Subject: [PATCH 01/11] :adhesive_bandage: Pass params by vals instead of by refs if we pass parameters by reference, it must be because we want to change them in the method and to keep those changes. This seems like a different situation, so we should be able to pass the objects by value. --- Sources/Model/IManager.cs | 6 ++-- Sources/Model/PlayerManager.cs | 10 +++--- Sources/Tests/Model_UTs/PlayerManagerTest.cs | 32 ++++++++++---------- 3 files changed, 24 insertions(+), 24 deletions(-) diff --git a/Sources/Model/IManager.cs b/Sources/Model/IManager.cs index ecc8ea9..2d7c13e 100644 --- a/Sources/Model/IManager.cs +++ b/Sources/Model/IManager.cs @@ -3,10 +3,10 @@ namespace Model { public interface IManager { - public T Add(ref T toAdd); + public T Add(T toAdd); public T GetOneById(int id); public IEnumerable GetAll(); - public T Update(ref T before, ref T after); - public void Remove(ref T toRemove); + public T Update(T before, T after); + public void Remove(T toRemove); } } diff --git a/Sources/Model/PlayerManager.cs b/Sources/Model/PlayerManager.cs index 93ab043..86eab00 100644 --- a/Sources/Model/PlayerManager.cs +++ b/Sources/Model/PlayerManager.cs @@ -26,7 +26,7 @@ namespace Model /// /// player to be added /// added player, or null if was null - public Player Add(ref Player toAdd) + public Player Add(Player toAdd) { if (toAdd != null) { @@ -76,17 +76,17 @@ namespace Model /// player to be updated /// player in the state that it needs to be in after the update /// updated player - public Player Update(ref Player before, ref Player after) + public Player Update(Player before, Player after) { - Remove(ref before); - return Add(ref after); + Remove(before); + return Add(after); } /// /// remove a player /// /// player to be removed - public void Remove(ref Player toRemove) + public void Remove(Player toRemove) { players.Remove(toRemove); } diff --git a/Sources/Tests/Model_UTs/PlayerManagerTest.cs b/Sources/Tests/Model_UTs/PlayerManagerTest.cs index b87d7d4..f34ff51 100644 --- a/Sources/Tests/Model_UTs/PlayerManagerTest.cs +++ b/Sources/Tests/Model_UTs/PlayerManagerTest.cs @@ -41,8 +41,8 @@ namespace Tests.Model_UTs // Act HashSet actual = new() { - playerManager.Add(ref alice), - playerManager.Add(ref bob) + playerManager.Add(alice), + playerManager.Add(bob) }; // Assert @@ -59,7 +59,7 @@ namespace Tests.Model_UTs // Act expected = null; - actual = playerManager.Add(ref expected); + actual = playerManager.Add(expected); // Assert Assert.Equal(expected, actual); @@ -89,7 +89,7 @@ namespace Tests.Model_UTs // Arrange PlayerManager playerManager = new(); Player player = new("Bob"); - playerManager.Add(ref player); + playerManager.Add(player); // Act Player result = playerManager.GetOneByName(name); @@ -104,7 +104,7 @@ namespace Tests.Model_UTs // Arrange PlayerManager playerManager = new(); Player player = new("Bob"); - playerManager.Add(ref player); + playerManager.Add(player); // Act Player result = playerManager.GetOneByName("Clyde"); @@ -123,7 +123,7 @@ namespace Tests.Model_UTs // Arrange PlayerManager playerManager = new(); Player expected = new("Bob"); - playerManager.Add(ref expected); + playerManager.Add(expected); // Act Player actual = playerManager.GetOneByName(name); @@ -138,10 +138,10 @@ namespace Tests.Model_UTs // Arrange PlayerManager playerManager = new(); Player p1 = new("Dylan"); - playerManager.Add(ref p1); + playerManager.Add(p1); // Act - playerManager.Remove(ref p1); + playerManager.Remove(p1); // Assert Assert.DoesNotContain(p1, playerManager.GetAll()); @@ -153,12 +153,12 @@ namespace Tests.Model_UTs // Arrange PlayerManager playerManager = new(); Player player = new("Dylan"); - playerManager.Add(ref player); + playerManager.Add(player); Player notPlayer = null; HashSet expected = new() { player }; // Act - playerManager.Remove(ref notPlayer); + playerManager.Remove(notPlayer); HashSet actual = (HashSet)playerManager.GetAll(); // Assert @@ -171,12 +171,12 @@ namespace Tests.Model_UTs // Arrange PlayerManager playerManager = new(); Player player = new("Dylan"); - playerManager.Add(ref player); + playerManager.Add(player); Player notPlayer = new("Eric"); HashSet expected = new() { player }; // Act - playerManager.Remove(ref notPlayer); + playerManager.Remove(notPlayer); HashSet actual = (HashSet)playerManager.GetAll(); // Assert @@ -189,11 +189,11 @@ namespace Tests.Model_UTs // Arrange PlayerManager playerManager = new(); Player oldPlayer = new("Dylan"); - playerManager.Add(ref oldPlayer); + playerManager.Add(oldPlayer); Player newPlayer = new("Eric"); // Act - playerManager.Update(ref oldPlayer, ref newPlayer); + playerManager.Update(oldPlayer, newPlayer); // Assert Assert.DoesNotContain(oldPlayer, playerManager.GetAll()); @@ -207,11 +207,11 @@ namespace Tests.Model_UTs string name = "Filibert"; PlayerManager playerManager = new(); Player oldPlayer = new(name); - playerManager.Add(ref oldPlayer); + playerManager.Add(oldPlayer); Player newPlayer = new(name); // Act - playerManager.Update(ref oldPlayer, ref newPlayer); + playerManager.Update(oldPlayer, newPlayer); // Assert Assert.Contains(oldPlayer, playerManager.GetAll()); From d36afce173d1d14f983418f8d14116f79e0fbff4 Mon Sep 17 00:00:00 2001 From: Alexis DRAI Date: Mon, 19 Sep 2022 14:55:27 +0200 Subject: [PATCH 02/11] =?UTF-8?q?=F0=9F=90=9B=20Add=20validation=20and=20d?= =?UTF-8?q?elegation=20to=20Player.Remove()?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Sources/Model/PlayerManager.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Sources/Model/PlayerManager.cs b/Sources/Model/PlayerManager.cs index 86eab00..b14c07f 100644 --- a/Sources/Model/PlayerManager.cs +++ b/Sources/Model/PlayerManager.cs @@ -88,7 +88,12 @@ namespace Model /// player to be removed public void Remove(Player toRemove) { - players.Remove(toRemove); + // delegating, making sure we find it even if different case etc. + if (toRemove != null) + { + Player realToRemove = GetOneByName(toRemove.Name); + players.Remove(realToRemove); + } } } } From a5483b38505cf351bff816bcc43cee11d6a8951e Mon Sep 17 00:00:00 2001 From: Alexis DRAI Date: Mon, 19 Sep 2022 14:57:37 +0200 Subject: [PATCH 03/11] =?UTF-8?q?=E2=9C=85=20Improve=20Update()=20test=20a?= =?UTF-8?q?fter=20bugfix?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Sources/Tests/Model_UTs/PlayerManagerTest.cs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/Sources/Tests/Model_UTs/PlayerManagerTest.cs b/Sources/Tests/Model_UTs/PlayerManagerTest.cs index f34ff51..a8bb664 100644 --- a/Sources/Tests/Model_UTs/PlayerManagerTest.cs +++ b/Sources/Tests/Model_UTs/PlayerManagerTest.cs @@ -200,15 +200,19 @@ namespace Tests.Model_UTs Assert.Contains(newPlayer, playerManager.GetAll()); } - [Fact] - public void TestUpdateDoesNothingIfSame() + [Theory] + [InlineData("Filibert", "filibert")] + [InlineData("Filibert", " fiLibert")] + [InlineData("Filibert", "FIlibert ")] + [InlineData(" Filibert", " filiBErt ")] + public void TestUpdateDiscreetlyUpdatesCaseAndIgnoresExtraSpaceIfOtherwiseSame(string n1, string n2) { // Arrange string name = "Filibert"; PlayerManager playerManager = new(); - Player oldPlayer = new(name); + Player oldPlayer = new(n1); playerManager.Add(oldPlayer); - Player newPlayer = new(name); + Player newPlayer = new(n2); // Act playerManager.Update(oldPlayer, newPlayer); @@ -216,6 +220,8 @@ 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 } } } From 0f3471cb60cb7765c15c378db76c0bcbfd4bb0a1 Mon Sep 17 00:00:00 2001 From: Alexis DRAI Date: Mon, 19 Sep 2022 14:58:05 +0200 Subject: [PATCH 04/11] =?UTF-8?q?=E2=9C=8F=EF=B8=8F=20Fix=20typo?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Sources/Tests/Model_UTs/PlayerManagerTest.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/Sources/Tests/Model_UTs/PlayerManagerTest.cs b/Sources/Tests/Model_UTs/PlayerManagerTest.cs index a8bb664..0e788d0 100644 --- a/Sources/Tests/Model_UTs/PlayerManagerTest.cs +++ b/Sources/Tests/Model_UTs/PlayerManagerTest.cs @@ -208,7 +208,6 @@ namespace Tests.Model_UTs public void TestUpdateDiscreetlyUpdatesCaseAndIgnoresExtraSpaceIfOtherwiseSame(string n1, string n2) { // Arrange - string name = "Filibert"; PlayerManager playerManager = new(); Player oldPlayer = new(n1); playerManager.Add(oldPlayer); From fb69b719b4677b346a68fe045dee8bac201c3f1c Mon Sep 17 00:00:00 2001 From: Alexis DRAI Date: Mon, 19 Sep 2022 14:59:03 +0200 Subject: [PATCH 05/11] =?UTF-8?q?=E2=9C=85=20Generalize=20from=20HashSets?= =?UTF-8?q?=20to=20IEnumerables=20or=20Collections?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Sources/Tests/Model_UTs/PlayerManagerTest.cs | 33 +++++++++----------- 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/Sources/Tests/Model_UTs/PlayerManagerTest.cs b/Sources/Tests/Model_UTs/PlayerManagerTest.cs index 0e788d0..f3f87dc 100644 --- a/Sources/Tests/Model_UTs/PlayerManagerTest.cs +++ b/Sources/Tests/Model_UTs/PlayerManagerTest.cs @@ -1,29 +1,25 @@ using Model; using System; using System.Collections.Generic; +using System.Collections.ObjectModel; using System.Linq; -using System.Text; -using System.Threading.Tasks; -using System.Xml.Linq; using Xunit; -using Xunit.Sdk; namespace Tests.Model_UTs { public class PlayerManagerTest { [Fact] - public void TestConstructorReturnsEmptyHashSet() + public void TestConstructorReturnsEmptyEnumerable() { // Arrange - PlayerManager playerManager; - HashSet expected; - HashSet actual; + PlayerManager playerManager = new(); + IEnumerable expected; + IEnumerable actual; // Act - playerManager = new(); - expected = new(); - actual = (HashSet)playerManager.GetAll(); + expected = new Collection(); + actual = playerManager.GetAll(); // Assert Assert.Equal(expected, actual); @@ -36,10 +32,10 @@ namespace Tests.Model_UTs PlayerManager playerManager = new(); Player alice = new("Alice"); Player bob = new("Bob"); - HashSet expected = new() { alice, bob }; // Act - HashSet actual = new() + Collection expected = new() { alice, bob }; + Collection actual = new() { playerManager.Add(alice), playerManager.Add(bob) @@ -59,7 +55,7 @@ namespace Tests.Model_UTs // Act expected = null; - actual = playerManager.Add(expected); + actual = playerManager.Add(expected);// Add() returns the added element if succesful // Assert Assert.Equal(expected, actual); @@ -155,11 +151,11 @@ namespace Tests.Model_UTs Player player = new("Dylan"); playerManager.Add(player); Player notPlayer = null; - HashSet expected = new() { player }; + IEnumerable expected = new Collection { player }; // Act playerManager.Remove(notPlayer); - HashSet actual = (HashSet)playerManager.GetAll(); + IEnumerable actual = playerManager.GetAll(); // Assert Assert.Equal(actual, expected); @@ -173,11 +169,11 @@ namespace Tests.Model_UTs Player player = new("Dylan"); playerManager.Add(player); Player notPlayer = new("Eric"); - HashSet expected = new() { player }; + IEnumerable expected = new Collection { player }; // Act playerManager.Remove(notPlayer); - HashSet actual = (HashSet)playerManager.GetAll(); + IEnumerable actual = playerManager.GetAll(); // Assert Assert.Equal(actual, expected); @@ -198,6 +194,7 @@ namespace Tests.Model_UTs // Assert Assert.DoesNotContain(oldPlayer, playerManager.GetAll()); Assert.Contains(newPlayer, playerManager.GetAll()); + Assert.True(playerManager.GetAll().Count() == 1); } [Theory] From e85b9e55509cb4050b98760a76c27869835bb0b8 Mon Sep 17 00:00:00 2001 From: Alexis DRAI Date: Mon, 19 Sep 2022 15:03:01 +0200 Subject: [PATCH 06/11] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Refactor,=20D.R.Y.,?= =?UTF-8?q?=20use=20"expected"=20and=20"actual"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Sources/Tests/Model_UTs/PlayerTest.cs | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/Sources/Tests/Model_UTs/PlayerTest.cs b/Sources/Tests/Model_UTs/PlayerTest.cs index efe8011..9ab7bb8 100644 --- a/Sources/Tests/Model_UTs/PlayerTest.cs +++ b/Sources/Tests/Model_UTs/PlayerTest.cs @@ -11,12 +11,14 @@ namespace Tests.Model_UTs { // Arrange Player player; + string expected = "Alice"; // Act - player = new("Alice"); + player = new(expected); + string actual = player.Name; // Assert - Assert.Equal("Alice", player.Name); + Assert.Equal(expected, actual); } [Fact] @@ -24,12 +26,14 @@ namespace Tests.Model_UTs { // Arrange Player player; + string expected = "Alice"; // Act - player = new("Alice "); + player = new(" Alice "); + string actual = player.Name; // Assert - Assert.Equal("Alice", player.Name); + Assert.Equal(expected, actual); } [Theory] @@ -159,10 +163,11 @@ namespace Tests.Model_UTs // Arrange Player p1; Player p2; + string name = "Elyse"; // Act - p1 = new("Elyse"); - p2 = new("Elyse"); + p1 = new(name); + p2 = new(name); // Assert Assert.True(p1.Equals(p2)); @@ -211,10 +216,11 @@ namespace Tests.Model_UTs // Arrange Player p1; Player p2; + string name = "Elyse"; // Act - p1 = new("Elyse"); - p2 = new("Elyse"); + p1 = new(name); + p2 = new(name); // Assert Assert.True(p1.GetHashCode().Equals(p2.GetHashCode())); From fb02307b289d01d818789f71ca0add831a8fc91b Mon Sep 17 00:00:00 2001 From: Alexis DRAI Date: Mon, 19 Sep 2022 21:27:18 +0200 Subject: [PATCH 07/11] :adhesive_bandage: Move validation to constructor, clarify with comments --- Sources/Model/Player.cs | 50 ++++++++++++++++++----------------------- 1 file changed, 22 insertions(+), 28 deletions(-) diff --git a/Sources/Model/Player.cs b/Sources/Model/Player.cs index a4a949e..5837f91 100644 --- a/Sources/Model/Player.cs +++ b/Sources/Model/Player.cs @@ -3,39 +3,33 @@ 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; } //TODO make setter private !!!!!!!!! + 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("player name may never be empty or null"); } - 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 +38,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 +47,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 } } } From 86597ef08f72a3f2b605be2195d36d0b7f18c860 Mon Sep 17 00:00:00 2001 From: Alexis DRAI Date: Mon, 19 Sep 2022 21:43:06 +0200 Subject: [PATCH 08/11] :adhesive_bandage: Remove old or useless TODOs --- Sources/Model/Player.cs | 2 +- Sources/Tests/Model_UTs/PlayerManagerTest.cs | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/Sources/Model/Player.cs b/Sources/Model/Player.cs index 5837f91..5953dc2 100644 --- a/Sources/Model/Player.cs +++ b/Sources/Model/Player.cs @@ -13,7 +13,7 @@ namespace Model /// /// a player's unique username /// - public string Name { get; private set; } //TODO make setter private !!!!!!!!! + public string Name { get; private set; } public Player(string name) { if (!String.IsNullOrWhiteSpace(name)) diff --git a/Sources/Tests/Model_UTs/PlayerManagerTest.cs b/Sources/Tests/Model_UTs/PlayerManagerTest.cs index f3f87dc..b4ac083 100644 --- a/Sources/Tests/Model_UTs/PlayerManagerTest.cs +++ b/Sources/Tests/Model_UTs/PlayerManagerTest.cs @@ -62,7 +62,6 @@ namespace Tests.Model_UTs Assert.DoesNotContain(expected, playerManager.GetAll()); } - // TODO update if we do implement it [Fact] public void TestGetOneByIdThrowsNotImplemented() { From 180c50b2c71d0059bf25090d482b0305bb6931b1 Mon Sep 17 00:00:00 2001 From: Alexis DRAI Date: Tue, 20 Sep 2022 11:37:57 +0200 Subject: [PATCH 09/11] :adhesive_bandage: Improve exception-throwing --- Sources/Model/Player.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Sources/Model/Player.cs b/Sources/Model/Player.cs index 5953dc2..32cb8b4 100644 --- a/Sources/Model/Player.cs +++ b/Sources/Model/Player.cs @@ -1,5 +1,4 @@ using System; -using System.Xml.Linq; namespace Model { @@ -20,7 +19,7 @@ namespace Model { Name = name.Trim(); } - else throw new ArgumentException("player name may never be empty or null"); + else throw new ArgumentException("param should not be null or blank", nameof(name)); } /// From 1ad5c7cf21f70183d01f76d528176c866982a4f6 Mon Sep 17 00:00:00 2001 From: Alexis DRAI Date: Tue, 20 Sep 2022 11:41:30 +0200 Subject: [PATCH 10/11] =?UTF-8?q?=F0=9F=9B=82=E2=9C=85=20Add=20null=20chec?= =?UTF-8?q?ks=20and=20exceptions,=20reflect=20change=20in=20UTs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Sources/Model/PlayerManager.cs | 43 ++++--- Sources/Tests/Model_UTs/PlayerManagerTest.cs | 115 ++++++++++++++++--- 2 files changed, 123 insertions(+), 35 deletions(-) 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); } } } From bbfe7291c4055713495bcf203f954d83bb758a7a Mon Sep 17 00:00:00 2001 From: Alexis DRAI Date: Tue, 20 Sep 2022 12:07:00 +0200 Subject: [PATCH 11/11] :adhesive_bandage: Fix code smell --- Sources/Model/PlayerManager.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Sources/Model/PlayerManager.cs b/Sources/Model/PlayerManager.cs index a23910f..806ef4b 100644 --- a/Sources/Model/PlayerManager.cs +++ b/Sources/Model/PlayerManager.cs @@ -82,7 +82,9 @@ namespace Model { if (player is null) { - throw new ArgumentNullException(nameof(player), "param should not be 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);