🛂✅ Add null checks and exceptions, reflect change in UTs

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
         /// <summary>
         /// a hashset of the players that this manager is in charge of
         /// <br/>
-        /// 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)
         /// </summary>
         private readonly HashSet<Player> 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");
         }
 
         /// <summary>
-        /// 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
         /// </summary>
         /// <param name="id"></param>
         /// <returns></returns>
         /// <exception cref="NotImplementedException"></exception>
         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");
         }
 
         /// <summary>
@@ -52,22 +49,23 @@ namespace Model
         /// that copy does not belong to this manager's players, so it should not be modified 
         /// </summary>
         /// <param name="name">a player's unique name</param>
-        /// <returns>player with said name</returns>
+        /// <returns>player with said name, <em>or null</em> if no such player was found</returns>
         public Player GetOneByName(string name)
         {
             if (!String.IsNullOrWhiteSpace(name))
             {
-                Player result = players.FirstOrDefault(p => p.Name.ToUpper().Equals(name.Trim().ToUpper()));
+                Player wanted = new(name);
-                if (result == null) return result; // will return null by default if no such player exists
+                Player result = players.FirstOrDefault(p => p.Equals(wanted));
-                return new(result); // THIS IS A COPY (using a copy constructor)
+                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));
         }
 
         /// </summary>
         /// 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
         /// </summary>
-        /// <returns>a readonly list of all this manager's players</returns>
+        /// <returns>a readonly enumerable of all this manager's players</returns>
         public IEnumerable<Player> GetAll() => players.AsEnumerable();
 
         /// <summary>
@@ -78,6 +76,15 @@ namespace Model
         /// <returns>updated player</returns>
         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
         /// <param name="toRemove">player to be removed</param>
         public void Remove(Player toRemove)
         {
-            // delegating, making sure we find it even if different case etc.
+            if (toRemove is null)
-            if (toRemove != null)
             {
-                Player realToRemove = GetOneByName(toRemove.Name); 
+                throw new ArgumentNullException(nameof(toRemove), "param should not be null");
-                players.Remove(realToRemove);
             }
+            // the built-in HashSet.Remove() method will use our redefined Equals(), using Name only
+            players.Remove(toRemove);
         }
     }
 }

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<ArgumentNullException>(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<ArgumentException>(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(new Player("Dylan"));
-            playerManager.Add(player);
-            Player notPlayer = null;
-            IEnumerable<Player> expected = new Collection<Player> { player };
 
             // Act
-            playerManager.Remove(notPlayer);
+            void action() => playerManager.Remove(null);
-            IEnumerable<Player> actual = playerManager.GetAll();
 
             // Assert
-            Assert.Equal(actual, expected);
+            Assert.Throws<ArgumentNullException>(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);
+            Assert.Equal(oldPlayer, newPlayer);
-            // uses Equals(), which is made to be case-insensitive
+        }
+
+        [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<ArgumentException>(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<ArgumentNullException>(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<ArgumentNullException>(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<ArgumentException>(action); // thrown by Player constructor
+            Assert.Contains(oldPlayer, playerManager.GetAll()); // still there
+            Assert.True(size1 == size2);
         }
     }
 }