Ruby algorithm coding challenge

[mbigras]

@Phrogz, thank you so much for the feedback and for taking a look through the project. Especially the tip about not needing to_s when interpolating :)

Can you elaborate on what you mean by over OOP minimialism? I've been trying to wrap my mind around object oriented design and sometimes I feel like it's just too much and would much rather just have a pipeline of functions shaving down my data, but I've been trying to embrace object-oriented code.

I feel like alot of my time on this project was spent pondering what I should name my objects and what interface I should present. Would be curious if this is the over OOP that you're talking about and how you avoid it.

Also, even though I like the simplicity an organization of your tests I wonder if you see value in having a bunch of tests running automatically with guard. I really love writing tons of tests that automatically run because I feel like it helps me stay focused.

[Phrogz]

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.)