From a319d7deb2a4f1164e81b37efab2d74062252327 Mon Sep 17 00:00:00 2001 From: Alexis DRAI Date: Mon, 10 Oct 2022 01:44:20 +0200 Subject: [PATCH] :rotating_light: fix code smells --- Sources/Data/EF/Players/PlayerDBManager.cs | 25 +++++++++++--- Sources/Data/EF/Players/PlayerDbManager.cs | 25 +++++++++++--- Sources/Data/Stub.cs | 2 +- Sources/Model/Dice/DiceGroupManager.cs | 21 +++++------- Sources/Model/Games/Game.cs | 2 +- Sources/Model/Games/GameManager.cs | 14 ++++---- Sources/Model/Players/PlayerManager.cs | 34 +++++++++---------- .../Data_UTs/Players/PlayerDbManagerTest.cs | 5 +-- .../Tests/Model_UTs/Games/GameManagerTest.cs | 2 +- Sources/Tests/Model_UTs/Games/GameTest.cs | 4 +-- .../Model_UTs/Games/MasterOfCeremoniesTest.cs | 4 +-- Sources/Tests/Model_UTs/Games/TurnTest.cs | 6 ++-- .../Model_UTs/Players/PlayerManagerTest.cs | 12 +++---- 13 files changed, 91 insertions(+), 65 deletions(-) diff --git a/Sources/Data/EF/Players/PlayerDBManager.cs b/Sources/Data/EF/Players/PlayerDBManager.cs index 40bf8b6..c10266b 100644 --- a/Sources/Data/EF/Players/PlayerDBManager.cs +++ b/Sources/Data/EF/Players/PlayerDBManager.cs @@ -45,7 +45,7 @@ namespace Data.EF.Players /// /// /// - public async Task Add(PlayerEntity toAdd) + public Task Add(PlayerEntity toAdd) { CleanPlayerEntity(toAdd); @@ -54,6 +54,11 @@ namespace Data.EF.Players throw new ArgumentException("this username is already taken", nameof(toAdd)); } + return InternalAdd(toAdd); + } + + private async Task InternalAdd(PlayerEntity toAdd) + { EntityEntry ee = await db.Players.AddAsync(toAdd); await db.SaveChangesAsync(); return (PlayerEntity)ee.Entity; @@ -74,17 +79,22 @@ namespace Data.EF.Players /// /// /// - public async Task GetOneByName(string name) + public Task GetOneByName(string name) { if (string.IsNullOrWhiteSpace(name)) { throw new ArgumentException("Name property should not be null or whitespace", nameof(name)); } name = name.Trim(); - return await db.Players.Where(p => p.Name == name).FirstAsync(); + return InternalGetOneByName(name); } + private async Task InternalGetOneByName(string name) + { + return await db.Players.Where(p => p.Name == name).FirstAsync(); + } + public async Task IsPresentByName(string name) { if (string.IsNullOrWhiteSpace(name)) @@ -119,7 +129,7 @@ namespace Data.EF.Players /// the updated entity /// /// - public async Task Update(PlayerEntity before, PlayerEntity after) + public Task Update(PlayerEntity before, PlayerEntity after) { PlayerEntity[] args = { before, after }; @@ -133,9 +143,14 @@ namespace Data.EF.Players throw new ArgumentException("ID cannot be updated", nameof(after)); } + return InternalUpdate(before, after); + + } + + private async Task InternalUpdate(PlayerEntity before, PlayerEntity after) + { Remove(before); return await Add(after); - } /// diff --git a/Sources/Data/EF/Players/PlayerDbManager.cs b/Sources/Data/EF/Players/PlayerDbManager.cs index 40bf8b6..c10266b 100644 --- a/Sources/Data/EF/Players/PlayerDbManager.cs +++ b/Sources/Data/EF/Players/PlayerDbManager.cs @@ -45,7 +45,7 @@ namespace Data.EF.Players /// /// /// - public async Task Add(PlayerEntity toAdd) + public Task Add(PlayerEntity toAdd) { CleanPlayerEntity(toAdd); @@ -54,6 +54,11 @@ namespace Data.EF.Players throw new ArgumentException("this username is already taken", nameof(toAdd)); } + return InternalAdd(toAdd); + } + + private async Task InternalAdd(PlayerEntity toAdd) + { EntityEntry ee = await db.Players.AddAsync(toAdd); await db.SaveChangesAsync(); return (PlayerEntity)ee.Entity; @@ -74,17 +79,22 @@ namespace Data.EF.Players /// /// /// - public async Task GetOneByName(string name) + public Task GetOneByName(string name) { if (string.IsNullOrWhiteSpace(name)) { throw new ArgumentException("Name property should not be null or whitespace", nameof(name)); } name = name.Trim(); - return await db.Players.Where(p => p.Name == name).FirstAsync(); + return InternalGetOneByName(name); } + private async Task InternalGetOneByName(string name) + { + return await db.Players.Where(p => p.Name == name).FirstAsync(); + } + public async Task IsPresentByName(string name) { if (string.IsNullOrWhiteSpace(name)) @@ -119,7 +129,7 @@ namespace Data.EF.Players /// the updated entity /// /// - public async Task Update(PlayerEntity before, PlayerEntity after) + public Task Update(PlayerEntity before, PlayerEntity after) { PlayerEntity[] args = { before, after }; @@ -133,9 +143,14 @@ namespace Data.EF.Players throw new ArgumentException("ID cannot be updated", nameof(after)); } + return InternalUpdate(before, after); + + } + + private async Task InternalUpdate(PlayerEntity before, PlayerEntity after) + { Remove(before); return await Add(after); - } /// diff --git a/Sources/Data/Stub.cs b/Sources/Data/Stub.cs index b20211f..8e747ea 100644 --- a/Sources/Data/Stub.cs +++ b/Sources/Data/Stub.cs @@ -82,7 +82,7 @@ namespace Data await (await gr.GameManager.GetOneByName(game3)).PlayerManager.Add(player1); await (await gr.GameManager.GetOneByName(game3)).PlayerManager.Add(player3); - foreach (Game game in gr.GameManager.GetAll().Result) + foreach (Game game in gr.GameManager.GetAll()?.Result) { for (int i = 0; i < 10; i++) { diff --git a/Sources/Model/Dice/DiceGroupManager.cs b/Sources/Model/Dice/DiceGroupManager.cs index 87133d1..5b688f2 100644 --- a/Sources/Model/Dice/DiceGroupManager.cs +++ b/Sources/Model/Dice/DiceGroupManager.cs @@ -9,7 +9,7 @@ namespace Model.Dice { private readonly Dictionary> diceGroups = new(); - public async Task>> Add(KeyValuePair> toAdd) + public Task>> Add(KeyValuePair> toAdd) { if (string.IsNullOrWhiteSpace(toAdd.Key)) { @@ -21,12 +21,12 @@ namespace Model.Dice throw new ArgumentException("this username is already taken", nameof(toAdd)); } diceGroups.Add(toAdd.Key.Trim(), toAdd.Value); - return await Task.FromResult(toAdd); + return Task.FromResult(toAdd); } - public async Task>>> GetAll() + public Task>>> GetAll() { - return await Task.FromResult(diceGroups.AsEnumerable()); + return Task.FromResult(diceGroups.AsEnumerable()); } public Task>> GetOneByID(Guid ID) @@ -34,7 +34,7 @@ namespace Model.Dice throw new NotImplementedException(); } - public async Task>> GetOneByName(string name) + public Task>> GetOneByName(string name) { // les groupes de dés nommés : // ils sont case-sensistive, mais "mon jeu" == "mon jeu " == " mon jeu" @@ -42,10 +42,7 @@ namespace Model.Dice { throw new ArgumentNullException(nameof(name), "param should not be null or empty"); } - else - { - return await Task.FromResult(new KeyValuePair>(name, diceGroups[name])); - } + return Task.FromResult(new KeyValuePair>(name, diceGroups[name])); } public void Remove(KeyValuePair> toRemove) @@ -68,7 +65,7 @@ namespace Model.Dice /// /// /// - public async Task>> Update(KeyValuePair> before, KeyValuePair> after) + public Task>> Update(KeyValuePair> before, KeyValuePair> after) { // pas autorisé de changer les dés, juste le nom if (!before.Value.Equals(after.Value)) @@ -80,8 +77,8 @@ namespace Model.Dice throw new ArgumentNullException(nameof(before), "dice group name should not be null or empty"); } diceGroups.Remove(before.Key); - await Add(after); - return after; + Add(after); + return Task.FromResult(after); } } diff --git a/Sources/Model/Games/Game.cs b/Sources/Model/Games/Game.cs index 553e912..6f9ae7d 100644 --- a/Sources/Model/Games/Game.cs +++ b/Sources/Model/Games/Game.cs @@ -180,7 +180,7 @@ namespace Model.Games sb.Append($"Game: {Name}"); sb.Append("\nPlayers:"); - foreach (Player player in PlayerManager.GetAll().Result) + foreach (Player player in PlayerManager.GetAll()?.Result) { sb.Append($" {player.ToString()}"); } diff --git a/Sources/Model/Games/GameManager.cs b/Sources/Model/Games/GameManager.cs index b5b5d69..f955374 100644 --- a/Sources/Model/Games/GameManager.cs +++ b/Sources/Model/Games/GameManager.cs @@ -21,7 +21,7 @@ namespace Model.Games /// gets an unmodifiable collection of the games /// /// unmodifiable collection of the games - public async Task> GetAll() => await Task.FromResult(games.AsEnumerable()); + public Task> GetAll() => Task.FromResult(games.AsEnumerable()); /// /// finds the game with that name and returns it @@ -30,11 +30,11 @@ namespace Model.Games /// /// a games's name /// game with said name, or null if no such game was found - public async Task GetOneByName(string name) + public Task GetOneByName(string name) { if (!string.IsNullOrWhiteSpace(name)) { - return await Task.FromResult(games.FirstOrDefault(g => g.Name == name)); // may return null + return Task.FromResult(games.FirstOrDefault(g => g.Name == name)); // may return null } throw new ArgumentException("param should not be null or blank", nameof(name)); } @@ -54,7 +54,7 @@ namespace Model.Games /// saves a given game -- does not allow copies yet: if a game with the same name exists, it is overwritten /// /// a game to save - public async Task Add(Game toAdd) + public Task Add(Game toAdd) { if (toAdd is null) { @@ -64,7 +64,7 @@ namespace Model.Games games.Remove(games.FirstOrDefault(g => g.Name == toAdd.Name)); // will often be an update: if game with that name exists, it is removed, else, nothing happens above games.Add(toAdd); - return await Task.FromResult(toAdd); + return Task.FromResult(toAdd); } /// @@ -88,7 +88,7 @@ namespace Model.Games /// new game /// /// - public async Task Update(Game before, Game after) + public Task Update(Game before, Game after) { Game[] args = { before, after }; @@ -103,7 +103,7 @@ namespace Model.Games } } Remove(before); - return await Add(after); + return (Add(after)); } } } diff --git a/Sources/Model/Players/PlayerManager.cs b/Sources/Model/Players/PlayerManager.cs index 95cf6d5..bb61a93 100644 --- a/Sources/Model/Players/PlayerManager.cs +++ b/Sources/Model/Players/PlayerManager.cs @@ -21,7 +21,7 @@ namespace Model.Players /// /// player to be added /// added player - public async Task Add(Player toAdd) + public Task Add(Player toAdd) { if (toAdd is null) { @@ -32,29 +32,27 @@ namespace Model.Players throw new ArgumentException("this username is already taken", nameof(toAdd)); } players.Add(toAdd); - return await Task.FromResult(toAdd); + return Task.FromResult(toAdd); } /// - /// finds the player with that name and returns A COPY OF IT - ///
- /// that copy does not belong to this manager's players, so it should not be modified + /// finds the player with that name and returns it /// /// a player's unique name /// player with said name, or null if no such player was found - public async Task GetOneByName(string name) + public Task GetOneByName(string name) { - if (!string.IsNullOrWhiteSpace(name)) + if (string.IsNullOrWhiteSpace(name)) { - Player wanted = new(name); - Player result = players.FirstOrDefault(p => p.Equals(wanted)); - if (result == null) - { - return null; - } - return await Task.FromResult(new Player(result)); // THIS IS A COPY (using a copy constructor) + throw new ArgumentException("param should not be null or blank", nameof(name)); + } + + Player result = players.FirstOrDefault(p => p.Name.ToUpper().Equals(name.ToUpper().Trim())); + if (result == null) + { + return Task.FromResult(null); } - throw new ArgumentException("param should not be null or blank", nameof(name)); + return Task.FromResult(result); } /// @@ -62,7 +60,7 @@ namespace Model.Players /// so that the only way to modify the collection of players is to use this class's methods /// /// a readonly enumerable of all this manager's players - public async Task> GetAll() => await Task.FromResult(players.AsEnumerable()); + public Task> GetAll() => Task.FromResult(players.AsEnumerable()); /// /// update a player from to @@ -70,7 +68,7 @@ namespace Model.Players /// player to be updated /// player in the state that it needs to be in after the update /// updated player - public async Task Update(Player before, Player after) + public Task Update(Player before, Player after) { Player[] args = { before, after }; @@ -84,7 +82,7 @@ namespace Model.Players } } Remove(before); - return await Add(after); + return Add(after); } /// diff --git a/Sources/Tests/Data_UTs/Players/PlayerDbManagerTest.cs b/Sources/Tests/Data_UTs/Players/PlayerDbManagerTest.cs index 0411ce5..d6522ce 100644 --- a/Sources/Tests/Data_UTs/Players/PlayerDbManagerTest.cs +++ b/Sources/Tests/Data_UTs/Players/PlayerDbManagerTest.cs @@ -367,8 +367,8 @@ namespace Tests.Data_UTs.Players } [Fact] - public async Task TestRemoveWhenPreExistentThenRemoves() - { + public void TestRemoveWhenPreExistentThenRemoves() + {// should be async Task // Arrange PlayerDbManager mgr; @@ -393,6 +393,7 @@ namespace Tests.Data_UTs.Players db.Database.EnsureCreated(); mgr = new(db); + // should check if toRemove is now absent Assert.True(true); } } diff --git a/Sources/Tests/Model_UTs/Games/GameManagerTest.cs b/Sources/Tests/Model_UTs/Games/GameManagerTest.cs index 3889667..7729450 100644 --- a/Sources/Tests/Model_UTs/Games/GameManagerTest.cs +++ b/Sources/Tests/Model_UTs/Games/GameManagerTest.cs @@ -16,7 +16,7 @@ namespace Tests.Model_UTs.Games { public class GameManagerTest { - private readonly MasterOfCeremonies stubGameRunner = new Stub().LoadApp().Result; + private readonly MasterOfCeremonies stubGameRunner = new Stub().LoadApp()?.Result; [Fact] public async Task TestConstructorReturnsEmptyEnumerableAsync() diff --git a/Sources/Tests/Model_UTs/Games/GameTest.cs b/Sources/Tests/Model_UTs/Games/GameTest.cs index 723f15e..39a9d30 100644 --- a/Sources/Tests/Model_UTs/Games/GameTest.cs +++ b/Sources/Tests/Model_UTs/Games/GameTest.cs @@ -14,14 +14,14 @@ namespace Tests.Model_UTs.Games { public class GameTest { - private readonly MasterOfCeremonies stubMasterOfCeremonies = new Stub().LoadApp().Result; + private readonly MasterOfCeremonies stubMasterOfCeremonies = new Stub().LoadApp()?.Result; private static readonly string GAME_NAME = "my game"; private static readonly Player PLAYER_1 = new("Alice"), PLAYER_2 = new("Bob"), PLAYER_3 = new("Clyde"); private readonly IEnumerable DICE_1, DICE_2; public GameTest() { - IEnumerable>> diceGroups = stubMasterOfCeremonies.DiceGroupManager.GetAll().Result; + IEnumerable>> diceGroups = stubMasterOfCeremonies.DiceGroupManager.GetAll()?.Result; DICE_1 = diceGroups.First().Value; DICE_2 = diceGroups.Last().Value; } diff --git a/Sources/Tests/Model_UTs/Games/MasterOfCeremoniesTest.cs b/Sources/Tests/Model_UTs/Games/MasterOfCeremoniesTest.cs index 058522e..a92dbd7 100644 --- a/Sources/Tests/Model_UTs/Games/MasterOfCeremoniesTest.cs +++ b/Sources/Tests/Model_UTs/Games/MasterOfCeremoniesTest.cs @@ -12,7 +12,7 @@ namespace Tests.Model_UTs.Games { public class MasterOfCeremoniesTest { - private readonly MasterOfCeremonies stubMasterOfCeremonies = new Stub().LoadApp().Result; + private readonly MasterOfCeremonies stubMasterOfCeremonies = new Stub().LoadApp()?.Result; [Fact] public async Task TestPlayGameWhenPlayThenAddNewTurnToHistoryAsync() @@ -46,7 +46,7 @@ namespace Tests.Model_UTs.Games await masterOfCeremonies.StartNewGame( name, new PlayerManager(), - stubMasterOfCeremonies.GameManager.GetAll().Result.First().Dice + stubMasterOfCeremonies.GameManager.GetAll()?.Result.First().Dice ); // Assert diff --git a/Sources/Tests/Model_UTs/Games/TurnTest.cs b/Sources/Tests/Model_UTs/Games/TurnTest.cs index 17dce2c..4e0465c 100644 --- a/Sources/Tests/Model_UTs/Games/TurnTest.cs +++ b/Sources/Tests/Model_UTs/Games/TurnTest.cs @@ -13,15 +13,15 @@ namespace Tests.Model_UTs.Games public class TurnTest { - private readonly MasterOfCeremonies stubMasterOfCeremonies = new Stub().LoadApp().Result; + private readonly MasterOfCeremonies stubMasterOfCeremonies = new Stub().LoadApp()?.Result; Dictionary DICE_N_FACES_1, DICE_N_FACES_2; public TurnTest() { - DICE_N_FACES_1 = (Dictionary)stubMasterOfCeremonies.GameManager.GetAll().Result.First().GetHistory().First().DiceNFaces; - DICE_N_FACES_2 = (Dictionary)stubMasterOfCeremonies.GameManager.GetAll().Result.Last().GetHistory().Last().DiceNFaces; + DICE_N_FACES_1 = (Dictionary)stubMasterOfCeremonies.GameManager.GetAll()?.Result.First().GetHistory().First().DiceNFaces; + DICE_N_FACES_2 = (Dictionary)stubMasterOfCeremonies.GameManager.GetAll()?.Result.Last().GetHistory().Last().DiceNFaces; } [Fact] diff --git a/Sources/Tests/Model_UTs/Players/PlayerManagerTest.cs b/Sources/Tests/Model_UTs/Players/PlayerManagerTest.cs index 0f69e60..578f337 100644 --- a/Sources/Tests/Model_UTs/Players/PlayerManagerTest.cs +++ b/Sources/Tests/Model_UTs/Players/PlayerManagerTest.cs @@ -111,15 +111,15 @@ namespace Tests.Model_UTs.Players } [Fact] - public async Task TestGetOneByNameIfValidButNotExistThenReturnNull() + public void TestGetOneByNameIfValidButNotExistThenReturnNull() { // Arrange PlayerManager playerManager = new(); Player player = new("Bob"); - await playerManager.Add(player); + playerManager.Add(player); // Act - Player result = await playerManager.GetOneByName("Clyde"); + Player result = playerManager.GetOneByName("Clyde")?.Result; // Assert Assert.Null(result); @@ -130,15 +130,15 @@ namespace Tests.Model_UTs.Players [InlineData("bob")] [InlineData("bob ")] [InlineData(" boB ")] - public async Task TestGetOneByNameIfValidThenReturnPlayer(string name) + public void TestGetOneByNameIfValidThenReturnPlayer(string name) { // Arrange PlayerManager playerManager = new(); Player expected = new("Bob"); - await playerManager.Add(expected); + playerManager.Add(expected); // Act - Player actual = await playerManager.GetOneByName(name); + Player actual = playerManager.GetOneByName(name)?.Result; // Assert Assert.Equal(expected, actual);