bowling scorecard challenge #390
Conversation
There was a problem hiding this comment.
The code has a bunch of tests that cover both state and behavior, making it pretty solid. It's easy to read and organised, plus it's got some nice error handling with clear explanations.
On the flip side, it could do a better job sticking to the Single Responsibility Principle since some methods are juggling a few tasks at once. Splitting them up would make the code even better.
Overall, it's a good piece of code, but with a few tweaks, it could be even better!
| return message | ||
| end | ||
|
|
||
| def add_bonus_frame(*args) |
There was a problem hiding this comment.
Based on the focus points provided to us I would suggest encapsulating the Frame logic into a seperate class. This would make it more modular and the bowlingscorer class would be only responsible for managing the Frames rather than the logic.
|
|
||
| ## Helper methods - Used in testing and not relevant to gameplay ## | ||
| def frames | ||
| @frames |
There was a problem hiding this comment.
I assume if you had multiple classes you would've used accessor methods to access these instance variables instead?
| it "fails if user inputs negative numbers" do | ||
| expect{bowl.add_frame(4,-5)}.to raise_error "Smells like invalid input" | ||
| end | ||
|
|
There was a problem hiding this comment.
Good fail scenarios but would suggest one for non numeric input, floating point and maybe other types but that's just me nitpicking.
| @player_score += @frames[-1].flatten.sum | ||
| end | ||
|
|
||
| def count_frame_score(index) |
There was a problem hiding this comment.
Would be good to split the responsibilities out here as it currently calculates the frame score and the special cases.
validating whether a user is allowed to get additional shots (after strike/sparing the 10th one) is assumed to be handled by the UI so havent been incorporated