bowling: add to track#675
Conversation
|
Thanks for tackling this @dawcars! :D I changed the name to "exercise-slug: dibs on implementing exercise". Once you're ready for a review feel free to change it to "exercise-slug: add to track". This is just to make it obvious to other contributors that you're working on an exercise, and obvious to reviewers once you're ready for a review :) |
|
Thanks @FridaTveit. That's really helpful. I will change the name when it is ready for review. Thanks for your help. :) |
|
No problem! :) |
7181d68 to
3e7f12d
Compare
|
Please can you check this is OK to add to the track. It should now be ready. Many thanks |
Smarticles101
left a comment
There was a problem hiding this comment.
Here's my review. These are mostly little comments on formatting that don't matter too much. The only thing that really has to be addressed is the comment I left on the starter implementation file.
| @@ -0,0 +1,11 @@ | |||
| public class BowlingGame { | |||
There was a problem hiding this comment.
Since the spot you have chosen to put this is >21, you can remove this file and replace it with a .keep file. (See the POLICIES.md file)
| private int frameScore(int frameIndex) { | ||
| return rolls.get(frameIndex) + rolls.get(frameIndex + 1); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
I noticed your formatting of this file is a little inconsistent, specifically on where you put your curly braces. Some are on the same line, but some are on the next. If you could just fix up the formatting that would be great. (In IntelliJ just press Ctrl+Shift+Alt+L to reformat the code)
| game = new BowlingGame(); | ||
| } | ||
|
|
||
| private void PlayGame(int[] rolls) |
There was a problem hiding this comment.
typically only constructors start with a capital letter. Instead of PlayGame it should be playGame
| @Test | ||
| public void shouldBeAbleToScoreAGameWithNoStrikesOrSpares() { | ||
| int[] rolls = { 3, 6, 3, 6, 3, 6, 3, 6, 3, 6, 3, 6, 3, 6, 3, 6, 3, 6, 3, 6 }; | ||
| PlayGame(rolls); |
There was a problem hiding this comment.
A newline would be nice to have in between these two lines in all the test cases
| public void aSpareFollowedByZerosIsWorthTenPoints() { | ||
| int[] rolls = { 6, 4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }; | ||
| PlayGame(rolls); | ||
| assertEquals(10, game.score()); |
There was a problem hiding this comment.
It might improve readability a little bit if the expected score was a variable instead
|
Thanks for your comments @Smarticles101. They are very helpful. I will make corrections based on your comments. If you find anything else that needs alteration, please let me know. |
|
Now that this has entered the review phase it's fine to leave it titled as "add to track" :) |
|
I have refactored changes as suggested by @Smarticles101 and uploaded to branch. All gradle tests are passing and I have rebased with the upstream. However I see that one of the checks is failing. Does anyone know why this might be? |
|
@dawcars I'm trying to look into why it's failing |
|
So far, I haven't been able to find out why it's failing. 😕 |
|
I've restarted it to see if that helps :) Sometimes the build fails for no apparent reason... But restarting it often helps :) |
|
[If this was branched off master before #674 was merged, that could be the cause.] |
|
Thanks @dawcars :) |
|
Thanks for your help with this @Smarticles101, @stkent and @FridaTveit. It's very much appreciated. :) |
I have started work on bowling exercise. Work in progress
Reviewer Resources:
Track Policies