-
Notifications
You must be signed in to change notification settings - Fork 48
review me #2
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?
review me #2
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| export class Game { | ||
| private _lastSymbol: string = ' '; | ||
| private _board: Board = new Board(); | ||
|
|
||
| public Play(symbol: string, x: number, y: number) : void { | ||
| //if first move | ||
| if (this._lastSymbol == ' ') { | ||
| //if player is X | ||
| if (symbol == 'O') { | ||
| throw new Error("Invalid first player"); | ||
| } | ||
| } | ||
| //if not first move but player repeated | ||
| else if (symbol == this._lastSymbol) { | ||
| 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 commentThe reason will be displayed to describe this comment to others. Learn more. This is an example of content coupling. You're dependent on the boards internal elements |
||
| throw new Error("Invalid position"); | ||
| } | ||
|
|
||
| // 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 commentThe reason will be displayed to describe this comment to others. Learn more. More content coupling |
||
| } | ||
|
|
||
| public Winner() : string { | ||
| //if the positions in first row are taken | ||
| if (this._board.TileAt(0, 0)!.Symbol != ' ' && | ||
| this._board.TileAt(0, 1)!.Symbol != ' ' && | ||
| this._board.TileAt(0, 2)!.Symbol != ' ') { | ||
| //if first row is full with same symbol | ||
| if (this._board.TileAt(0, 0)!.Symbol == | ||
| this._board.TileAt(0, 1)!.Symbol && | ||
| this._board.TileAt(0, 2)!.Symbol == this._board.TileAt(0, 1)!.Symbol) { | ||
| return this._board.TileAt(0, 0)!.Symbol; | ||
| } | ||
| } | ||
|
|
||
| //if the positions in first row are taken | ||
| if (this._board.TileAt(1, 0)!.Symbol != ' ' && | ||
| this._board.TileAt(1, 1)!.Symbol != ' ' && | ||
| this._board.TileAt(1, 2)!.Symbol != ' ') { | ||
| //if middle row is full with same symbol | ||
| if (this._board.TileAt(1, 0)!.Symbol == | ||
| this._board.TileAt(1, 1)!.Symbol && | ||
| this._board.TileAt(1, 2)!.Symbol == | ||
| this._board.TileAt(1, 1)!.Symbol) { | ||
| return this._board.TileAt(1, 0)!.Symbol; | ||
| } | ||
| } | ||
|
|
||
| //if the positions in first row are taken | ||
| if (this._board.TileAt(2, 0)!.Symbol != ' ' && | ||
| this._board.TileAt(2, 1)!.Symbol != ' ' && | ||
| this._board.TileAt(2, 2)!.Symbol != ' ') { | ||
| //if middle row is full with same symbol | ||
| if (this._board.TileAt(2, 0)!.Symbol == | ||
| this._board.TileAt(2, 1)!.Symbol && | ||
| this._board.TileAt(2, 2)!.Symbol == | ||
| this._board.TileAt(2, 1)!.Symbol) { | ||
| return this._board.TileAt(2, 0)!.Symbol; | ||
| } | ||
| } | ||
|
|
||
| return ' '; | ||
| } | ||
| } | ||
|
|
||
| interface Tile | ||
| { | ||
| X: number; | ||
| Y: number; | ||
| Symbol: string; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From typscript docs: "Aliasing doesn’t actually create a new type - it creates a new name to refer to that type. Aliasing a primitive is not terribly useful, though it can be used as a form of documentation." Having to learn a protocol is much cumbersome than having the compiler/IDE help you with the fulfilling of the methods with a type hieracy. |
||
| } | ||
|
|
||
| class Board | ||
| { | ||
| private _plays : Tile[] = []; | ||
|
|
||
| constructor() | ||
| { | ||
| for (let i = 0; i < 3; i++) | ||
| { | ||
| for (let j = 0; j < 3; j++) | ||
| { | ||
| const tile : Tile = {X :i, Y:j, Symbol:" "}; | ||
| this._plays.push(tile); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public TileAt(x: number, y: number): Tile { | ||
| return this._plays.find((t:Tile) => t.X == x && t.Y == y)! | ||
| } | ||
|
|
||
| public AddTileAt(symbol: string, x: number, y: number) : void | ||
| { | ||
| const tile : Tile = {X :x, Y:y, Symbol:symbol}; | ||
|
|
||
| this._plays.find((t:Tile) => t.X == x && t.Y == y)!.Symbol = 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 Game class is coupled to this specific implementation of a Board. By creating the
Boardclass here, we cannot switch out the board for a different implementation.We could use depenency injection instead, where we pass the Board class to the constructor of Game.
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.
Injecting/reutilising an element could be a good design decision imo. This allow less tech debt and even a direct monetary return if scaling both
classesin Lambdas having different workload. Though it may be interesting to see if Winner would be calling/coupling Board way to much.