-
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
Add Dice game #2
base: main
Are you sure you want to change the base?
Conversation
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.
Wrote some comments.
src/Abstractions/BaseTypes.cs
Outdated
public static DiceBoard Empty(int size) => EmptyCache.GetOrAdd(size, size1 => new DiceBoard(size1)); | ||
|
||
public int Size { get; } | ||
public Dictionary<int, string[]> Cells { 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.
Most of Game* types are immutable, this dictionary is mutable.
src/Abstractions/BaseTypes.cs
Outdated
get { | ||
var cellIndex = GetCellIndex(r, c); | ||
if (cellIndex < 0 || cellIndex >= Cells.Count) | ||
return new string[] {"lightblue", "lightblue", "lightblue", "lightblue"}; |
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.
IMO it's a bad idea to use string as underlying player color or something like this. Maybe an enum?
src/Abstractions/BaseTypes.cs
Outdated
|
||
public DiceBoard(int size) | ||
{ | ||
var defaultValue = "lightblue"; |
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.
Not sure what "lightblue" means :) Color? Empty cell?
src/Host/HostSettings.cs
Outdated
@@ -17,7 +17,7 @@ public class HostSettings | |||
|
|||
// DBs | |||
public string UsePostgreSql { get; set; } = | |||
"Server=localhost;Database=board_games_dev;Port=5432;User Id=postgres;Password=Fusion.0.to.1"; | |||
"Server=localhost;Database=board_games_dev;Port=5432;User Id=postgres;Password=pg051825"; |
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.
IMO you shouldn't change the default password that matches the password from docker-compose.yml
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 just use the original docker-compose.yml?
src/Host/Startup.cs
Outdated
@@ -75,6 +75,8 @@ public void ConfigureServices(IServiceCollection services) | |||
HostSettings = tmpServices.GetRequiredService<HostSettings>(); | |||
|
|||
// DbContext & related services | |||
|
|||
// HostSettings.UseSqlite = true; |
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 commented out piece should be removed.
@attribute [MatchFor(typeof(DiceEngine), ComponentScopes.GameRules)] | ||
@inherits GameRulesBase | ||
|
||
<p><b>Rules</b>: Get to the finish line first.</p> |
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.
Ok, I'll write a bit more details here :)
@@ -20,6 +20,7 @@ public override void Use() | |||
{ | |||
// Game engines | |||
Services.TryAddEnumerable(ServiceDescriptor.Singleton<IGameEngine, GomokuEngine>()); | |||
Services.TryAddEnumerable(ServiceDescriptor.Singleton<IGameEngine, DiceEngine>()); |
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.
FYI this changed in the latest game verison.
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.
Check out the attributes on GomokuEngine & copy them to get this working.
src/Abstractions/CharBoard.cs
Outdated
public static DiceBoard Empty(int size) => EmptyCache.GetOrAdd(size, size1 => new DiceBoard(size1)); | ||
|
||
public int Size { get; } | ||
public ImmutableDictionary<int, Cell> Cells { 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 don't need a dict if you use 100% index range from 0 to N :) You need an array :)
src/Abstractions/CharBoard.cs
Outdated
|
||
public enum Colors | ||
{ | ||
Blue, |
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 would rather use CellStates vs Colors. Color is meaningless, but cell state - e.g. "BoostCell" - is meaninful.
src/Abstractions/CharBoard.cs
Outdated
public int Size { get; } | ||
public ImmutableDictionary<int, Cell> Cells { get; } | ||
|
||
public struct Cell |
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 thing definitely requires some serialization-related optimizations. Or maybe DiceBoard.
src/Abstractions/CharBoard.cs
Outdated
public struct Cell | ||
{ | ||
public string Background; | ||
public string[] Colors; |
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.
Why cell has multiple colors?
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.
4 colors for all players (blue, green, red, yellow)
src/Abstractions/CharBoard.cs
Outdated
{ | ||
public string Background; | ||
public string[] Colors; | ||
public double[] Opacities; |
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.
Same for opacities?
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.
Each cell has 1 background color and 4 "color + opacity" units.
src/Abstractions/CharBoard.cs
Outdated
Size = size; | ||
var builder = ImmutableDictionary.CreateBuilder<int, Cell>(); | ||
for (int i = 0; i < size * size; i++) { | ||
if (i == 10 || i == 27 || i == 44) { // ForwardStep cells |
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 make this random.
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.
Random can make infinite loop.
src/Abstractions/Games/Dice.cs
Outdated
var playerScore = oldPlayerScore += move.Value; | ||
state.Steps[state.PlayerIndex] += 1; | ||
var playerSteps = state.Steps[state.PlayerIndex]; | ||
if (playerScore == 10 || playerScore == 27 || playerScore == 44) { |
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 is kinda weird - i.e. I'd use board to check exactly this thing.
No description provided.