-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Idan domain ucc03 0 #24
Conversation
…ervice layer), controller(comm layer)
…ost of them related to not meeded MVC
# Conflicts: # Poker.BE/Poker.BE.Domain.Tests/Game/PlayerTests.cs # Poker.BE/Poker.BE.Domain.Tests/Game/RoomTests.cs # Poker.BE/Poker.BE.Domain/Game/Player.cs # Poker.BE/Poker.BE.Domain/Game/Room.cs # Poker.BE/Poker.BE.Domain/Game/Round.cs # Poker.BE/Poker.BE.Domain/Game/Turn.cs # Poker.BE/Poker.BE.Domain/Game/Wallet.cs # Poker.BE/Poker.BE.Domain/Poker.BE.Domain.csproj # Poker.BE/Poker.BE.Domain/Utility/Exceptions/NotEnoughMoneyException.cs # Poker.BE/Poker.BE.Domain/Utility/Exceptions/NotEnoughPlayersException.cs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
basically everything is fine, just see my comments/questions.
those are small changes.
this.CurrentHand = null; | ||
this.Name = new GameConfig().Name; | ||
} | ||
|
||
public Player CreatePlayer() | ||
{ | ||
var result = new Player(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the player suppose to have a name as I understood.
so maybe the CreatePlayer() method should get his name from the user's name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm.. yeah i thought about it. It because of the equals override i did. I can add a condition to the method that if the name is empty, do a shallow compare instead of deep. It should solve the problem. I'll add a test for that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
{ | ||
// TODO | ||
throw new NotImplementedException(); | ||
creator = new Player(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
player should get the name from the user's name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Player has a nickname. User has a name. They can be edited separately
#region Constants | ||
public static readonly int NCHAIRS_IN_ROOM = 10; | ||
#endregion | ||
|
||
|
||
#region Fields | ||
private int index; | ||
private bool isBusy; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure np
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
private double _minimumBet; | ||
private string _name; | ||
private GamePreferences _preferences; | ||
#endregion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're missing the "chip policy" option that tells you how many chips do the player gets when joining the game.
this is NOT related to buy-in policy... it's a different thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I leave it to 'game preferences' as according to our design
@@ -7,32 +7,199 @@ namespace Poker.BE.Domain.Game | |||
public class GameConfig | |||
{ | |||
|
|||
#region Fields | |||
private int _maxNumberOfActivePlayers; | |||
private int _maxNumberOfPlayers; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we care only about the min/max number of players at the table, aka "active players"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Min/max numbers are for room configuration. Active Players is a property in Room class. Updated according to room state
/// </summary> | ||
public int MinNumberOfPlayers { get; set; } | ||
public int MaxNumberOfPlayers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can remove this MaxNumberOfPlayers property
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. We need it
#region Properties | ||
public State CurrentState { get; set; } | ||
public Wallet Wallet { get { return _wallet; } } | ||
public double WalletValue { get { return _wallet.Value; } private set { _wallet.Value = value; } } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to choose:
we have the amountOfMoney field in the Wallet class and we have the value field in its father class.
we don't need both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. Issue #25 wallet
get { return config.BuyInCost; } | ||
set { config.BuyInCost = value; } | ||
} | ||
#endregion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need all those properties? doesn't it enough to have the GamePrefences object related to it that holds all of this information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By UC of create room.. yup, we do need them
|
||
#endregion | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we don't need this class at all...
let's use just one kind of currency.
the chip-to-money conversion will be calculated by the game preferences in the specific room (just divide the chip policy by the buy-in policy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I differ. :) Let's talk on this f2f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This related to #25
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job here idan! Can you explain me too what do we need this class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arieltobiana see Issue #25 wallet
@@ -6,16 +6,21 @@ | |||
|
|||
namespace Poker.BE.Domain.Game | |||
{ | |||
public class Wallet | |||
public class Wallet : Utility.MoneyStorage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to decide if we need the MoneyStorage class.
see comment in that class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#25 wallet
#region Constructors | ||
public RoomController() | ||
{ | ||
service = new Service.Services.RoomsService(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new Service.Services? seems like bad smell or maybe I miss something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you miss something.
[TestInitialize] | ||
public void Before() | ||
{ | ||
gameCenter = GameCenter.Instance; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gamecenter is singleton? nice good idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. as we design. credit for all.
var expRoom2 = gameCenter.CreateNewRoom(level, config, out creator2).Name = "test room 2"; | ||
|
||
//Act | ||
var actual = gameCenter.FindRoomsByCriteria(25); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is criteria 25?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
integer bind to level parameter - look at the method definition
Assert.AreEqual(0, expRoom.ActivePlayers.Count); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good tests! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
//{ | ||
|
||
//} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe can we delete comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arieltobiana - this tests are for Connect()
Disconnect()
methods of User
class. that you didn't make any test for them and no documentation that explain what are their purpose. look at the //TODO: ...
RaisePreconditions(this.CurrentPlayer.Wallet.amountOfMoney + LiveBets[CurrentPlayer] - TotalRaise); | ||
Raise(this.CurrentPlayer.Wallet.amountOfMoney + LiveBets[CurrentPlayer] - TotalRaise); | ||
RaisePreconditions(this.CurrentPlayer.Wallet.AmountOfMoney + LiveBets[CurrentPlayer] - TotalRaise); | ||
Raise(this.CurrentPlayer.Wallet.AmountOfMoney + LiveBets[CurrentPlayer] - TotalRaise); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mabrouk about the code convention 👍
newPartialPot.PlayersClaimPot.Add(player); | ||
} | ||
|
||
lastPartialPot.PlayersClaimPot.Remove(CurrentPlayer); | ||
|
||
newPartialPot.AmountToClaim = amountToAdd + lastPlayerCurrentBet; | ||
lastPartialPot.AmountToClaim -= newPartialPot.AmountToClaim; | ||
newPartialPot.Value = lastPartialPot.Value - (lastPartialPot.AmountToClaim * lastPartialPot.PlayersClaimPot.Count); | ||
lastPartialPot.Value = lastPartialPot.AmountToClaim * lastPartialPot.PlayersClaimPot.Count; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that I understand the link between pot and partialpot. Do we have some pot tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomer314 - the pot expert 😆
return obj.GetHashCode(); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@idanizi what is the purpose of this class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i've added ///<summery>
|
||
#endregion | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job here idan! Can you explain me too what do we need this class?
} | ||
#endregion | ||
|
||
}// class | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good job! I'm impressed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answered & Done
i've pass through @tomer314 and @arieltobiana reviews - many thanks!
return obj.GetHashCode(); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i've added ///<summery>
|
||
#endregion | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arieltobiana see Issue #25 wallet
} | ||
#endregion | ||
|
||
}// class | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
#region Constructors | ||
public RoomController() | ||
{ | ||
service = new Service.Services.RoomsService(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you miss something.
[TestInitialize] | ||
public void Before() | ||
{ | ||
gameCenter = GameCenter.Instance; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. as we design. credit for all.
{ | ||
throw new PlayerModeException("Unable to stand up: Player already a spectator."); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deck = new Deck(); | ||
chairs = new Chair[Chair.NCHAIRS_IN_ROOM]; | ||
chairs = new Chair[NCHAIRS_IN_ROOM]; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes.
@@ -98,9 +173,50 @@ public Room(Player creator, GamePreferences preferences) : this(creator) | |||
Preferences = preferences; | |||
} | |||
|
|||
public Room(Player creator, GameConfig config) : this(creator, config.Preferences) | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just for the creation, to add it to "passive players". he doesn't have spacial privileges
this.CurrentHand = null; | ||
this.Name = new GameConfig().Name; | ||
} | ||
|
||
public Player CreatePlayer() | ||
{ | ||
var result = new Player(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
newPartialPot.PlayersClaimPot.Add(player); | ||
} | ||
|
||
lastPartialPot.PlayersClaimPot.Remove(CurrentPlayer); | ||
|
||
newPartialPot.AmountToClaim = amountToAdd + lastPlayerCurrentBet; | ||
lastPartialPot.AmountToClaim -= newPartialPot.AmountToClaim; | ||
newPartialPot.Value = lastPartialPot.Value - (lastPartialPot.AmountToClaim * lastPartialPot.PlayersClaimPot.Count); | ||
lastPartialPot.Value = lastPartialPot.AmountToClaim * lastPartialPot.PlayersClaimPot.Count; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomer314 - the pot expert 😆
UCC03 - Rooms Management - Domain Layer