refactor exercise word-search #420

pheanex · 2017-03-01T21:50:11Z

rewrite tests so that users have more freedom of implementation
update example-implementation to fit test-interface
reduce skeleton to minimal as discussed in Skeleton files should be included in all problems to make it easier #272

rootulp · 2017-03-02T20:28:31Z

exercises/word-search/example.py

+def find_stop(row, column, word, puzzle):
+    directions = [(0, 1), (0, -1), (1, 0), (1, 1), (1, -1), (-1, 0), (-1, 1), (-1, -1)]
+    for d in directions:
+        row_nr, column_nr = row, column


Does nr refer to number? If so, why not make this explicit with row_number and column_number?

yes it refers to number, i always try to find the sweet spot between overview/compactness and expressiveness ;-). I'll just update it if you think the gain in expressiveness is worth the loss in compactness and brevity. I'll also then change "char" into "character"

It's a small nitpick and it's entirely up to you. I was confused by the _nr and I'm only pointing it out in the hope that others can avoid similar confusion.

okay then it absolutely makes sense to change it, thank you for the feedback/review.

* rewrite tests so that users have more freedom of implementation * update example-implementation to fit test-interface * reduce skeleton to minimal as discussed in exercism#272

behrtam · 2017-03-08T08:30:01Z

exercises/word-search/word_search_test.py

-                  'jalaycalmp\n'
-                  'clojurermt')
-        self.example = WordSearch(puzzle)
+    puzzle = ['jefblpepre',


Why did you change the input format? I mean it's only a split(\n) away ... I just would like to know the reasoning behind this change.

Both ways are fine and both ways are seen in other tracks. C# uses a string with \n as delimiter (code) while Go uses a list of strings (code).

behrtam · 2017-03-08T08:36:53Z

exercises/word-search/word_search_test.py

-            self.example.search('clojure'),
-            (Point(0, 9), Point(6, 9))
-        )
+        self.assertEqual(search(WordSearchTests.puzzle, 'clojure'), ((9, 0), (9, 6)))


Why did you reverse the x, y order? Normally coordinate systems (wiki) use (X, Y) notation if I'm not mistaken.

pheanex · 2017-03-22T06:40:28Z

Is there any way we can get a third reviewer? @behrtam any suggestions?

behrtam · 2017-03-22T09:53:44Z

Maybe @rootulp has some time to look at it again. (Or do you want a fourth?)
It would help every other reviewer if you could explain your motivations behind the changes I asked you about.

pheanex · 2017-03-22T17:40:24Z

I thought i did explain my motivation (see above). If there is still something that you think i should motivate further, please specify.

behrtam · 2017-03-22T17:54:24Z

I appreciate the effort you have put into improving this exercise from a heavy class based exercise into a lighter flexible function based exercise and hope my maybe annoying questions about some details have not discouraged you.

You switched (0, 9) -> (9, 0) so from the typical (X, Y) to (Y, X) order which is just a small detail, but why? The only explanation I can find ist "freedom of implementation" which for me is not relevant for the X/Y order and for the list vs string input topic.

pheanex · 2017-03-22T18:02:52Z

No problem.
I tried to say it with "I had to change the order to make it consistent with the naming (row/column)."
and because it is a more straight forward representation to me. That is why i wanted to know how others felt about it. (Hence the question with the o x o x o o x ...).
Also i threw out the newlines (\n) since i thought it is cleaner and simpler without them. (So to just keep whats absolutely necessary and i felt newlines were not absolutely necessary.)

behrtam · 2017-04-17T20:41:03Z

So, the canonical test data is on it's way: exercism/problem-specifications#755

behrtam · 2017-06-09T06:59:02Z

Maybe we could show off namedtuples in this exercise?

>>> from collections import namedtuple
>>> Cell = namedtuple('Cell', ['row', 'column'])
>>> start = Cell(2, 2)
>>> end = Cell(row=2, column=12)

pheanex · 2017-06-10T10:56:16Z

I guess with canonical test data we can close this

* Create common working area * Extract Concepts from v2 exercise: reverse-string (exercism#657) * Create reverse-string.md * Update reverse-string.md * Add Concepts from v2 exercise: variable-length-quantity (exercism#503) * Add first concepts group * Improved concepts as per PR review * Adds concept from binary-search-tree (exercism#501) * Add initial list (exercism#463) First pass concepts for `allergies` to address exercism#460 * Initial list of concepts (exercism#462) First pass list of concepts to address exercism#459 * Add Concepts for v2 exercise: phone-number (exercism#420) * Add phone-number Python concepts * Small update to index access and slice topics. * Add notes from review. - more information about classes, inheritance - flesh out privacy, public and non-public - clarify wording around iterables and index/slice access * One more note about brackets and strings. * Add Concepts for v2 exercise: hamming (exercism#418) * Add concepts for hamming * Add note about tuple unpacking. * Add notes about polymorphism, builtins, and dunder methods. * Some whitespace fixes. * [WIP] `clock` exercise concepts. (exercism#395) * Extract Concepts from v2 exercise: markdown (exercism#540) * Initial commit for markdown exercise concepts. * Concept starter for markdown * Added detail to Markdown concepts * Final edits before harmonization Final Markdown edits before we merge and harmonize. * Add Concepts for v2 exercise: matrix (exercism#416) * `matrix` exercise concepts (issue exercism#386) First pass of concepts for `matrix ` exercise in python. Pretty sure this is too detailed, but wanted to get something for review before proceeding with additional exercises. * Edits to better match exercism#290 Formatting Edited concepts to better match the formatting of issue exercism#290 * Typo correction * added title * Extract Concepts from v2 exercise: rna-transcription (exercism#520) * Beginning of Concepts for rna-transcription * More detailed concepts for rna-trranscription More detailed concepts for rna-transcription exrcise. * Added title * Extract Concepts from v2 exercise: robot-simulator (exercism#538) * Beginning of concepts for robot-simulator. * WIP Concepts * Additional detail for concepts * Detail third pass Third pass on adding concept detail. * Additional detail for concepts. * Edits per PR Feedback Numerous spelling corrections. Additional edits to address comments from last review. * [WIP] Concept implementation instructions (exercism#665) * Adds instructions for exercise implementation * Adds correction as per PR reviews * Harmonize, part 1 (exercism#705) * fix relative links in references/README.md * First pass at harmonization Shifts all documents to a common format, adds minimal link tagging to the "concept" currently listed in each file. These will really need multiple more passes, as they diverge from each other even when describing the same topic. Many extraneous topics have crept in, added in an "aspirational" fashion to the exercises; we may need to trim some of that. * Pulling in examples from BethanyG * [WIP] Extracted concept unification (exercism#793) * Unification of extracted concepts * Typos and duplicates remove * Duplicates concept unification * Concepts have now links to original file * Update languages/reference/README.md Co-Authored-By: Erik Schierboom <erik_schierboom@hotmail.com> Co-authored-by: khoivan88 <33493502+khoivan88@users.noreply.github.com> Co-authored-by: David G <davidgerva@gmail.com> Co-authored-by: Ashley Drake <a.l.drake713@gmail.com> Co-authored-by: Pedro Romano <pedro@paparomeo.net> Co-authored-by: BethanyG <BethanyG@users.noreply.github.com> Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>

pheanex mentioned this pull request Mar 1, 2017

Add word-search exercise #380

Merged

pheanex force-pushed the refactor-word-search branch from e66b447 to e6010e9 Compare March 1, 2017 21:59

rootulp reviewed Mar 2, 2017

View reviewed changes

refactor exercise word-search

b001abc

* rewrite tests so that users have more freedom of implementation * update example-implementation to fit test-interface * reduce skeleton to minimal as discussed in exercism#272

pheanex force-pushed the refactor-word-search branch from e6010e9 to b001abc Compare March 2, 2017 20:38

behrtam reviewed Mar 8, 2017

View reviewed changes

pheanex closed this Jun 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

refactor exercise word-search #420

refactor exercise word-search #420

Uh oh!

pheanex commented Mar 1, 2017

Uh oh!

rootulp Mar 2, 2017

Uh oh!

pheanex Mar 2, 2017 •

edited

Loading

Uh oh!

rootulp Mar 2, 2017

Uh oh!

pheanex Mar 2, 2017

Uh oh!

behrtam Mar 8, 2017 •

edited

Loading

Uh oh!

behrtam Mar 8, 2017 •

edited

Loading

Uh oh!

pheanex commented Mar 22, 2017

Uh oh!

behrtam commented Mar 22, 2017

Uh oh!

pheanex commented Mar 22, 2017

Uh oh!

behrtam commented Mar 22, 2017

Uh oh!

pheanex commented Mar 22, 2017

Uh oh!

behrtam commented Apr 17, 2017

Uh oh!

behrtam commented Jun 9, 2017

Uh oh!

pheanex commented Jun 10, 2017

Uh oh!

Uh oh!

Uh oh!

refactor exercise word-search #420

refactor exercise word-search #420

Uh oh!

Conversation

pheanex commented Mar 1, 2017

Uh oh!

rootulp Mar 2, 2017

Choose a reason for hiding this comment

Uh oh!

pheanex Mar 2, 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!

rootulp Mar 2, 2017

Choose a reason for hiding this comment

Uh oh!

pheanex Mar 2, 2017

Choose a reason for hiding this comment

Uh oh!

behrtam Mar 8, 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!

behrtam Mar 8, 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!

pheanex commented Mar 22, 2017

Uh oh!

behrtam commented Mar 22, 2017

Uh oh!

pheanex commented Mar 22, 2017

Uh oh!

behrtam commented Mar 22, 2017

Uh oh!

pheanex commented Mar 22, 2017

Uh oh!

behrtam commented Apr 17, 2017

Uh oh!

behrtam commented Jun 9, 2017

Uh oh!

pheanex commented Jun 10, 2017

Uh oh!

Uh oh!

pheanex Mar 2, 2017 •

edited

Loading

behrtam Mar 8, 2017 •

edited

Loading

behrtam Mar 8, 2017 •

edited

Loading