-
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
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 is an example of content coupling. You're dependent on the boards internal elements
| @@ -0,0 +1,103 @@ | |||
| export class Game { | |||
| private _lastSymbol: string = ' '; | |||
| private _board: Board = new Board(); | |||
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 Board class 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 classes in Lambdas having different workload. Though it may be interesting to see if Winner would be calling/coupling Board way to much.
|
|
||
| // 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.
More content coupling
| { | ||
| X: number; | ||
| Y: number; | ||
| Symbol: string; |
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.
Symbol is externally coupled
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.
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.
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 I would suggest on thinking of how the design vocabulary could be applied on serverless functions architechture.
| @@ -0,0 +1,103 @@ | |||
| export class Game { | |||
| private _lastSymbol: string = ' '; | |||
| private _board: Board = new Board(); | |||
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 classes in Lambdas having different workload. Though it may be interesting to see if Winner would be calling/coupling Board way to much.
| { | ||
| X: number; | ||
| Y: number; | ||
| Symbol: string; |
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.
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.
No description provided.