-
Notifications
You must be signed in to change notification settings - Fork 8
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
Branch02 #5
base: main
Are you sure you want to change the base?
Conversation
src/Abstractions/CharBoard.cs
Outdated
@@ -61,6 +61,8 @@ protected virtual bool PrintMembers(StringBuilder builder) | |||
|
|||
public int GetCellIndex(int r, int c) => r * Size + c; | |||
|
|||
public int GetCardIndex(int r, int c) => r * 9 + c; |
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 function is unused.
src/Abstractions/GameEngine.cs
Outdated
? p with { Score = p.Score + score} | ||
: p).ToImmutableList(), | ||
}; | ||
public abstract string Id { get; } |
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 broke formatting here.
src/Abstractions/Games/CardsPoint.cs
Outdated
{ } | ||
|
||
[Service, ServiceAlias(typeof(IGameEngine), IsEnumerable = true)] | ||
public class PointEngine : GameEngine<PointState, PointMove> |
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'd rename this to PointGameEngine, PointGameState, and PointGameMove - PointXxx is a bit confusing.
src/Host/Program.cs
Outdated
@@ -13,7 +13,7 @@ | |||
// Looks like there is no better way to set _default_ URL | |||
builder.Sources.Insert(0, new MemoryConfigurationSource() { | |||
InitialData = new Dictionary<string, string>() { | |||
{WebHostDefaults.ServerUrlsKey, "http://localhost:5030"}, | |||
{WebHostDefaults.ServerUrlsKey, "http://localhost:5035"}, |
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 line should be reverted.
src/UI/Dice/DicePlay.razor
Outdated
<tbody> | ||
<tr> | ||
@for (int i = 0; i < 2; i++) | ||
{ |
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.
Statement brackets should be on the same line.
@@ -0,0 +1,185 @@ | |||
@attribute [MatchFor(typeof(DiceEngine), ComponentScopes.GamePlay)] |
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.
General comment: nesting is pretty huge in this file. I'd probably extract some sub-components here.
src/UI/Dice/DicePlay.razor
Outdated
|
||
private Task MoveAsync() | ||
{ | ||
diceValue = GetRandomDigit(); |
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.
Hehe :) This stuff should normally happen on server, right? I mean, if your client can submit any digit it likes, pretty sure it won't be a fair game at all.
src/UI/Dice/DicePlay.razor
Outdated
private Task MoveAsync() | ||
{ | ||
diceValue = GetRandomDigit(); | ||
var move = new DiceMove(MyPlayerIndex, diceValue); |
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.
Yep, diceValue shouldn't be a part of DiceMove - it should be generated on server side when you process the move there.
src/UI/Dice/DicePlay.razor
Outdated
private Color GetDiceColor(int playerIndex) | ||
=> playerIndex == MyPlayerIndex ? Color.Success : Color.Danger; | ||
|
||
private int GetRandomDigit() |
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.
So this should be a part of DiceEngine, I guess.
|
||
} | ||
|
||
<style> |
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.
Is it common to put style in the end of component?
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.
Overall, it looks very good now. Requested a couple small changes.
@@ -30,7 +30,7 @@ public abstract class GameEngine<TGameState, TGameMove> : IGameEngine | |||
|
|||
public virtual Game Create(Game game) => game; | |||
public abstract Game Start(Game game); | |||
Game IGameEngine.Move(Game game, GameMove move) => Move(game, (TGameMove) move); | |||
Game IGameEngine.Move(Game game, GameMove move) => Move(game, (TGameMove)move); |
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.
Space is needed here.
@@ -16,6 +20,7 @@ public ClientServicesModule(IServiceCollection services, IServiceProvider module | |||
|
|||
public override void Use() | |||
{ | |||
|
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.
Hmm, are you sure empty line is needed here?
@attribute [MatchFor(typeof(TicEngine), ComponentScopes.GameRules)] | ||
@inherits GameRulesBase | ||
|
||
Just a Tic Tac Toe game... Ordindary. |
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.
"An ordinary one."
No description provided.