Skip to content

Add word-search exercise - #380

Merged
behrtam merged 2 commits into
exercism:masterfrom
behrtam:implement-word-search
Feb 21, 2017
Merged

Add word-search exercise#380
behrtam merged 2 commits into
exercism:masterfrom
behrtam:implement-word-search

Conversation

@behrtam

Copy link
Copy Markdown
Contributor

I don't really like the example solution, but it works. Also I'm not sure if it is the right way to provide the class Point this way.

https://github.com/exercism/x-common/blob/master/exercises/word-search/description.md

@behrtam
behrtamforce-pushed the implement-word-search branch from b850c51 to 4827317CompareOctober 31, 2016 16:09
@arcuru

Copy link
Copy Markdown
Contributor

I like providing the Point class. It could've been done with just having them return the coords as a tuple, but this way is more explicit.

LGTM 👍

@behrtam
behrtam merged commit 5e2df01 into exercism:masterFeb 21, 2017
@pheanex

Copy link
Copy Markdown
Contributor

Hey @behrtam, I've got two questions on this:

@behrtam

Copy link
Copy Markdown
ContributorAuthor

Well it's four months away, but I assume I did it that way because the test suit relies on the class Point. This is probably a unpleasant result of my example implementation which I normally write first.

Maybe if we would instead test with tuples that represent a point, we would not need a dedicated class for that, which also means we could reduce the skeleton file and the people solving this problem would have more freedom.

@pheanex

Copy link
Copy Markdown
Contributor

Ah ok that makes sense, thank you.
If its okay with you, I'll try to refactor it a bit and send a PR 😄

@pheanex

Copy link
Copy Markdown
Contributor

Okay i created PR #420, please review.
Please note: I also changed the order of point tuples from coordinate-based (x,y) to a (at least to my mind) more intuitive (row,colum) notation. (If you prefer the coordinate-system, i'll update it).

@behrtam
behrtam deleted the implement-word-search branch March 22, 2017 15:17
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@behrtam@arcuru@pheanex