-
Notifications
You must be signed in to change notification settings - Fork 48
Implemented TicTacToe feature #3
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
base: pullrequests
Are you sure you want to change the base?
Conversation
| throw new Error("Invalid next player"); | ||
| } | ||
| //if not first move but play on an already played tile | ||
| else if (this._board.TileAt(x, y).Symbol != ' ') { |
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._board.TileAt(x, y).Symbol content coupling
| // update game state | ||
| this._lastSymbol = symbol; | ||
| this._board.AddTileAt(symbol, x, y); | ||
| } |
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 breaking rule one of the SOLID Princple (single responsibility)
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 can list the responsibilities with a name, you can never go bad with an apporach like that, I've came with this two, surely there can be better ones.
Responsibilites in this class:
A) decide winner
B) keep track of the turn
|
|
||
| public Winner() : string { | ||
| //if the positions in first row are taken | ||
| if (this._board.TileAt(0, 0)!.Symbol != ' ' && |
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.
content coupling here ^
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 internal content is exposed so it can be modified by external classes
We not only can access to the internals of the board but also we have to learn a protocol to interpret it. The convention ' ' means empty tile could really be any convention '_'.
WencesLlobet
left a comment
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.
If you want to go deeper in the subjects of last thursday I would suggest on thinking of more inapplicable situations (design pattern, personal project...) like the builder pattern we discussed.
| // update game state | ||
| this._lastSymbol = symbol; | ||
| this._board.AddTileAt(symbol, x, y); | ||
| } |
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 can list the responsibilities with a name, you can never go bad with an apporach like that, I've came with this two, surely there can be better ones.
Responsibilites in this class:
A) decide winner
B) keep track of the turn
|
|
||
| public Winner() : string { | ||
| //if the positions in first row are taken | ||
| if (this._board.TileAt(0, 0)!.Symbol != ' ' && |
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 internal content is exposed so it can be modified by external classes
We not only can access to the internals of the board but also we have to learn a protocol to interpret it. The convention ' ' means empty tile could really be any convention '_'.
No description provided.