🔀 Merge #52 into main

Sources/Model/Player.cs

@@ -1,41 +1,34 @@
 using System;
-using System.Xml.Linq;
 
 namespace Model
 {
+    /// <summary>
+    /// A player for the purpose of a game of dice, consists of a name
+    /// <br/>
+    /// Two players are equal for Equals() and GetHashCode() if they have the same name, once trimmed and case-insensitive
+    /// </summary>
     public sealed class Player : IEquatable<Player>
     {
-        public string Name
+        /// <summary>
+        /// a player's unique username
+        /// </summary>
+        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;
-        }
-
+        /// <summary>
+        /// this is a copy contructor
+        /// </summary>
+        /// <param name="player">a player object to be copied</param>
         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
         }
     }
 }

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()));
-                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));
         }
 
         /// </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,17 @@ 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(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
         /// <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 != 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);
         }
     }
 }

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<ArgumentNullException>(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<ArgumentException>(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<Player> expected = new Collection<Player> { player };
+            playerManager.Add(new Player("Dylan"));
 
             // Act
-            playerManager.Remove(notPlayer);
-            IEnumerable<Player> actual = playerManager.GetAll();
+            void action() => playerManager.Remove(null);
 
             // Assert
-            Assert.Equal(actual, expected);
+            Assert.Throws<ArgumentNullException>(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<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);
         }
     }
 }