Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

word-search: add canonical data #755

Merged
merged 1 commit into from
Apr 28, 2017
Merged

word-search: add canonical data #755

merged 1 commit into from
Apr 28, 2017

Conversation

stkent
Copy link
Contributor

@stkent stkent commented Apr 17, 2017

Closes #591.

Notes/thoughts:

  • Single grid is used for all tests. The likelihood that a practitioner will hard-code the actual search logic in any way is low.
  • Grid used for all tests matches the updated grid from the exercise description.md; Go track will need to update that to match this spec.
  • Test cases for each of the 8 possible word directions are cumulative, which allows better matching of the problem statement ("given a puzzle and a list of words") and a more explicit prompt to "build up" the search capability piece-by-piece. All existing tracks would need to change to accommodate this new input structure, I believe.

Feedback welcome! 👂

"screeaumgr",
"alxhpburyi",
"jalaycalmp",
"clojurermt"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell there's no way to share this repeated input between tests.

Copy link
Member

@petertseng petertseng Apr 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed - before the schema, we had repeated inputs forgrep and pov. The schema did not support that, so in #699 we discussed and decided not to add the support.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for the link; given that discussion, I think the duplication here is acceptable.

"elixir"
],
"expected": [
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It occurs to me that the only indication that the first element in this array indicates the position of "clojure" and the second that of "elixir" is convention.

I'm sorry that I cannot speak for all tracks, but I know that in Go the tests at least arrange such that output is a map whose keys are strings and corresponding values are what we have here (start/end column/row).

So I would like to ask about that. It seems unclear to leave it up to convention.

Another interesting item is that it appears the Go track is the only track that asks for multiple words at once. It's true that that matches the description. If it has to be that way (rather than simply changing the description), I would like to make sure that interface is solid before proceeding

"ruby",
"haskell"
],
"expected": -1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it becomes the case that the output can allow easy identification of which start/end pair corresponds to which word, I didn't find it too necessary to error on the entire output if just a single word is missing.

@stkent
Copy link
Contributor Author

stkent commented Apr 21, 2017

@petertseng great observations! I'm at a conference right now, but I'll push some updates based on your suggestions over the weekend :o)

@stkent stkent force-pushed the word-search-canonical-data branch from 4dbf0d6 to 127a97f Compare April 21, 2017 16:05
@stkent
Copy link
Contributor Author

stkent commented Apr 21, 2017

Another interesting item is that it appears the Go track is the only track that asks for multiple words at once. It's true that that matches the description. If it has to be that way (rather than simply changing the description), I would like to make sure that interface is solid before proceeding

Not only does this canonical data better match the existing description, but the end result also better reflects what I would consider to be a general word search solver (I've never seen a word search puzzle that asks you to locate a single word 😄). That's the reason I elected to adjust the input structure in the data rather than adjust the problem description. ⚖️

@stkent
Copy link
Contributor Author

stkent commented Apr 21, 2017

Updated!

Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, verified with

require 'json'
require_relative '../../verify'

DIRS = [-1, 0, 1].product([-1, 0, 1]).tap { |x| x.delete([0, 0]) }.freeze

def find_word(grid, word, starts)
  # one-index.
  coord = ->(y, x) {{"row" => y + 1, "column" => x + 1}}

  starts.product(DIRS).each { |(y, x), (dy, dx)|
    next unless word.each_char.with_index.all? { |c, i|
      cy = y + dy * i
      cx = x + dx * i
      next false if cy < 0 || cx < 0
      next false unless (row = grid[cy])
      row[cx] == c
    }
    d = word.size - 1
    return {
      "start" => coord[y, x],
      "end" => coord[y + d * dy, x + d * dx],
    }
  }
  nil
end

def find_words(grid, words)
  starts = grid.flat_map.with_index { |row, y|
    (0...row.size).map { |x| [y, x] }
  }.group_by { |y, x| grid[y][x] }
  words.zip(words.map { |word| find_word(grid, word, starts[word[0]]) }).to_h
end

json = JSON.parse(File.read(File.join(__dir__, 'canonical-data.json')))

verify(json['cases'], property: 'search') { |c|
  find_words(c['grid'], c['wordsToSearchFor'])
}

@stkent
Copy link
Contributor Author

stkent commented Apr 24, 2017

DIRS = [-1, 0, 1].product([-1, 0, 1]).tap { |x| x.delete([0, 0]) }.freeze

😍

@Insti Insti merged commit 86d6338 into master Apr 28, 2017
@Insti Insti deleted the word-search-canonical-data branch April 28, 2017 20:18
@Insti
Copy link
Contributor

Insti commented Apr 28, 2017

Thanks @stkent ❤️

emcoding pushed a commit that referenced this pull request Nov 19, 2018
Seems appropriate to link to the below section on disabling all skips, but I couldn't find examples of that happening elsewhere. Also not sure if tying the link in to github's style of id generation is desirable.
Sign up for free to 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.

4 participants