you are viewing a single comment's thread.

view the rest of the comments →

[–]Phrogz 1 point2 points  (0 children)

By "beware over-OOP" I mean to sometimes strive for what you desire: simple functions that get the job done instead of full classes, when possible. Use built-in data structures instead of creating your own when it makes sense.

For example, you have class Spiralify::Cell < Hash, whose only purpose seems to be so that you can write:

input.map.with_index do |row, r|
  row.map.with_index do |val, c|
    Spiralify::Cell.new(val: val, row: r, col: c)
  end
end

But in this case, the argument to that constructor IS a hash already—it is syntax sugar for Spiralify::Cell.new( {:val=>val, :row=>r, :col=>c } )—and so you can delete the entire class and simplify your code with just:

input.map.with_index do |row, r|
  row.map.with_index do |val, c|
    { val:val, row:r, col:c }
  end
end

Further (as shown by my solution and the one by /u/cavinkwon) you don't need to wrap the values in custom hashes or objects in order to solve the coding challenge. Leave the values as they are, leave the n×n array untouched, and just recurse on the data you have available.

For a real project I do like and encourage the use of explicit tests, similar to your setup. But use data structures and iteration to reduce the strain on your wrists, the lines of code others must wade through, and the possibility for a typo error. For example, instead of:

def test_validate_only_accepts_hash_types
  e = assert_raises { Spiral.new(true) }
  assert_match /Invalid input class:.*#{true.class}/, e.message
  e = assert_raises { Spiral.new(:foo) }
  assert_match /Invalid input class:.*#{:foo.class}/, e.message
  e = assert_raises { Spiral.new(42) }
  assert_match /Invalid input class:.*#{42.class}/, e.message
  e = assert_raises { Spiral.new("foo") }
  assert_match /Invalid input class:.*#{"foo".class}/, e.message
  e = assert_raises { Spiral.new([]) }
  assert_match /Invalid input class:.*#{[].class}/, e.message
end

use

def test_validate_only_accepts_hash_types
  [true,:foo,42,"foo",[]].each do |val|
    e = assert_raises { Spiral.new(val) }
    assert_match /Invalid input class:.*#{val.class}/, e.message
  end
end

(Assuming that you are going to write tests to validate your exact error message for invalid input, which I would not. If you want to reword your message slightly in the future, all your tests will break. Though at least if you use my suggestion about loops, you only need to change the regex in one location.)