[WIP] Implement exercise bowling by thecouchcoder · Pull Request #790 · exercism/python

thecouchcoder · 2017-10-10T12:43:43Z

https://github.com/exercism/problem-specifications/tree/master/exercises/bowling

ilya-khadykin · 2017-10-11T19:07:15Z

@aes421 build failed due to some code style issues:

2.65s$ flake8
./exercises/bowling/bowling_test.py:73:1: E305 expected 2 blank lines after class or function definition, found 1
./exercises/bowling/example.py:13:9: E265 block comment should start with '# '
./exercises/bowling/example.py:25:17: E265 block comment should start with '# '

ilya-khadykin · 2017-10-12T06:41:58Z

@aes421 you forgot to add the exercise to config.json:

-> An implementation for 'bowling' was found, but config.json does not reference this exercise.

thecouchcoder · 2017-10-12T14:15:44Z

Hey @m-a-ge I'm actually not done writing the test/implementation of this exercise yet. I left the config stuff until last for me previous PR, but I can go ahead and do it now if you'd like.

ilya-khadykin · 2017-10-12T14:42:59Z

Ah, sorry, my bad. Ping me when you're done.

Side note: I don't usually leave comments in WIP PRs, but some people forget to remove the tag and it's unclear whether PR is done or not

…l WIP

thecouchcoder · 2017-10-14T18:59:37Z

@m-a-ge done with this one. Let me know what you'd like to see changed.

ilya-khadykin

Good work overall!

I left comments for you to address.
And I suggest you spend some time on refactoring to make your code a bit better

ilya-khadykin · 2017-10-14T19:08:09Z

exercises/bowling/.gitignore

@@ -0,0 +1 @@
+.cache/


I would advise not to include this in repo .gitignore. You can use your local one for this purpose

ilya-khadykin · 2017-10-14T19:10:23Z

exercises/bowling/bowling_test.py

+
+from example import BowlingGame
+
+


Could you please also leave a comment stating what version of canonical-data.json the tests were adopted as discussed in #784 if aplicable?
The format is:

# Tests adapted from `problem-specifications//canonical-data.json` @ v1.0.0

ilya-khadykin · 2017-10-14T19:17:04Z

exercises/bowling/bowling_test.py

+    def test_should_be_able_to_score_a_game_with_all_zeros(self):
+        rolls = [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
+
+        self.game = BowlingGame()


I would also suggest introduce a setUp method and create an instance of BowlingGame only once

def setUp(self): self.game= BowlingGame()

ilya-khadykin · 2017-10-14T19:22:50Z

exercises/bowling/example.py

+        self.bonusRollsSeen = 0
+
+    def roll(self, pins):
+        if (self.isBonusRoll()):


probably just:

if self.isBonusRoll(): # do stuff

This increases readability

ilya-khadykin · 2017-10-14T19:24:00Z

exercises/bowling/example.py

+                raise ValueError
+
+        # open a new frame if the last one has been closed
+        if (self.currentFrame.isOpen() is False):


if not self.currentFrame.isOpen():

ilya-khadykin · 2017-10-14T19:38:27Z

exercises/bowling/example.py

+                self.rolls[1] = roll
+                self.open = False
+
+    def isOpen(self):


this doesn't look right, method name doesn't correspond to its actual function

I'm not sure what you mean on this one. you call isOpen to find out if the frame is open (true) or not (false)

ilya-khadykin · 2017-10-14T19:38:58Z

exercises/bowling/example.py

+
+
+class Frame(object):
+


It would be great to add docstring

ilya-khadykin · 2017-10-14T19:39:29Z

exercises/bowling/example.py

+
+    def isBonusRoll(self):
+        # if we've already seen all
+        if (len(self.rolls) >= NUM_FRAMES):


refactor to oneliner

ilya-khadykin · 2017-10-14T19:41:01Z

exercises/bowling/example.py

+
+
+class BowlingGame(object):
+    def __init__(self):


docstring here would be nice 📓

ilya-khadykin · 2017-10-14T19:47:54Z

exercises/bowling/bowling_test.py

+
+        self.game = BowlingGame()
+
+        for roll in rolls:


Probably something like:

[self.game.roll(roll) for roll in rolls]

And the code is duplicated in every test which is not a good thing, should be refactored

cmccandless

There should be a solution template bowling.py included as well. See solution template for complex-numbers as an example.

cmccandless · 2017-10-24T18:08:07Z

exercises/bowling/bowling_test.py

@@ -0,0 +1,261 @@
+import unittest
+
+from example import BowlingGame


should be from bowling import BowlingGame

thecouchcoder · 2017-11-07T20:09:49Z

Thanks for the feedback! I just got back from a holiday, and I'm hoping to have time to work on finishing this one later this week.

stale · 2017-11-28T20:56:15Z

This issue has been automatically marked as abandoned because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

thecouchcoder · 2017-11-29T20:31:47Z

I think I've got everything fixed up. Let me know what you think. @m-a-ge @cmccandless

cmccandless · 2017-11-29T21:16:52Z

exercises/bowling/bowling_test.py

@@ -1,258 +1,189 @@
+# Tests adapted from `problem-specifications//canonical-data.json` @ v1.0.1


For consistency with the other exercises, could you please move this line like so:

from bowling import BowlingGame # Tests adapted from `problem-specifications//canonical-data.json` @ v1.0.1 class BowlingTests(unittest.TestCase): ...

Please note the number of blank lines before and after.

stale · 2017-12-22T19:39:22Z

This issue has been automatically marked as abandoned because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

cmccandless · 2018-01-23T14:34:19Z

@aes421 Merged; thanks for working on this!

thecouchcoder added 6 commits October 10, 2017 07:40

Implement exercise bowling

c382caa

bowling: created the files

ad21e2a

bowling: implemented test for an all 0 game

1e16f9e

bowling: passing all zeros test

5b29088

bowling tested and implemented logic for spares

028ce96

bowling: bonus roll implemented

5b61ca4

thecouchcoder added 3 commits October 11, 2017 15:24

bowling: heavily refactored code. Now passing 9 tests

4527240

bowling: tested and implemeneted first 14 tests

1785b1f

fix travis issues with line length

73c311f

thecouchcoder force-pushed the bowling branch from b9a5449 to 73c311f Compare October 12, 2017 03:10

thecouchcoder added 11 commits October 12, 2017 18:41

bowling: setup config

3eb75d1

bowling: passing 19 tests

9fb30a7

bowling: fixed error in config.json

090300d

bowling: heavily refactoring code to implement frame logic during rol…

ff70fd5

…l WIP

bowling: still alot of refactoring going on and alot is broken

293bf44

bowling: still refactoring and still alot of errors, but closer

0cd0fc1

bowling: nearly done refactoring

3f1777e

bowling: finally back to passing all tests like before refactor

55c3c6c

bowling: added a few more tests and cleaned up after refactor

53b6e52

bowling: passes all tests

be6cf1c

bowling: fixing travis issue

91d1cf0

ilya-khadykin suggested changes Oct 14, 2017

View reviewed changes

cmccandless suggested changes Oct 24, 2017

View reviewed changes

stale bot added the abandoned label Nov 28, 2017

bowling: fixes as requested

6034a95

stale bot removed the abandoned label Nov 29, 2017

thecouchcoder added 5 commits November 29, 2017 13:58

bowling: fixes as requested

511732a

bowling: Travis fixes

5298cbb

bowling: Travis fixes

d3c349d

bowling: Travis fixes

833dce2

bowling: Travis fixes

1cf050f

cmccandless approved these changes Nov 29, 2017

View reviewed changes

bowling: fixes as requested

5f8b980

stale bot added the abandoned label Dec 22, 2017

cmccandless requested a review from ilya-khadykin December 26, 2017 17:12

stale bot removed the abandoned label Dec 26, 2017

cmccandless added 3 commits January 15, 2018 12:17

Merge branch 'master' into bowling

dca0f0e

Merge branch 'master' into bowling

95b7666

Merge branch 'master' into bowling

de5a2bb

cmccandless merged commit f6ae46b into exercism:master Jan 23, 2018

		@@ -0,0 +1,261 @@
		import unittest

		from example import BowlingGame

		@@ -1,258 +1,189 @@
		# Tests adapted from `problem-specifications//canonical-data.json` @ v1.0.1

		@@ -0,0 +1 @@
		.cache/

Uh oh!

Conversation

thecouchcoder commented Oct 10, 2017

Uh oh!

ilya-khadykin commented Oct 11, 2017

Uh oh!

ilya-khadykin commented Oct 12, 2017

Uh oh!

thecouchcoder commented Oct 12, 2017

Uh oh!

ilya-khadykin commented Oct 12, 2017

Uh oh!

thecouchcoder commented Oct 14, 2017

Uh oh!

ilya-khadykin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilya-khadykin Oct 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilya-khadykin Oct 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmccandless left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thecouchcoder commented Nov 7, 2017

Uh oh!

stale bot commented Nov 28, 2017

Uh oh!

thecouchcoder commented Nov 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Dec 22, 2017

Uh oh!

cmccandless commented Jan 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ilya-khadykin left a comment •

edited

Loading

ilya-khadykin Oct 14, 2017 •

edited

Loading

ilya-khadykin Oct 14, 2017 •

edited

Loading