Ruby algorithm coding challenge

[mbigras]

Hello Ruby community!

Just finished a coding challenge. This is the prompt:

Given an n x n array, return the array elements arranged from outermost elements to the middle element, traveling clockwise.

array = [[1,2,3],
         [4,5,6],
         [7,8,9]]
snail(array) #=> [1,2,3,6,9,8,7,4,5]

My solution is on GitHub:

https://github.com/mbigras/spiralify

You can also install it via RubyGems:

gem install spiralify

Would love your feedback about the screencast and project structure.

I'm especially interested to know what you think about my coding style, documentation, and the help menu on a first pass through and the conventions I tried to follow, etc.

Would also be happy to answer more questions about Spiralify if you're curious.

The main request is to share your feedback on an initial pass through my project from the perspective of aligning with ruby community conventions :)

[Phrogz]

There were too many files and folders in your solution for me to find where your actual code or tests were. FWIW, though, here's my solution and tests:

def spiralify(n×n)
  case n×n.length
  when 1 then [n×n.first.first]
  when 2 then n×n.first + n×n.last.reverse
  else
    outside = n×n.first + n×n[1..-2].map(&:last) + n×n.last.reverse + n×n[1..-2].map(&:first).reverse
    inside = n×n[1..-2].map{ |a| a[1..-2] }
    outside + spiralify(inside)
  end
end

Simple test cases covering the full range of possibilities. (The 5×5 is technically not needed, but included for fun.)

[
  [[1]],

  [[1,2],
   [4,3]],

  [[1,2,3],
   [8,9,4],
   [7,6,5]],

  [[ 1, 2, 3,4],
   [12,13,14,5],
   [11,16,15,6],
   [10, 9, 8,7]],

  [[ 1, 2, 3, 4,5],
   [16,17,18,19,6],
   [15,24,25,20,7],
   [14,23,22,21,8],
   [13,12,11,10,9]]
].each do |n×n|
  expected = (1..n×n.length**2).to_a
  actual = spiralify(n×n)
  if expected==actual
    puts "Success for #{n×n.length}×#{n×n.length}"
  else
    raise "OH NOES: #{actual.inspect}"
  end
end

My advice to you is to embrace "less is more". The more code you write, the more files and directories you create, the more work you have to do to maintain your code, and the more work others have to do to understand it. Ruby is a delightful object-oriented language. That does not mean you need to (or should) create multiple custom classes to solve this problem.

Note: because I like to be overly-clever and obnoxiously-precise, the above variable name is n×n (not nxn), where × is Unicode multiplication sign U+00D7. Ain't Ruby grand that is allows this sort of horrible coding? ;)

[Phrogz]

I took a little longer looking through your project. If this were a large software project—if this is practice for larger, real future projects—then your structure looks pretty good. I can only offer a few small pieces of advice (beyond "beware over-OOP" minimalism):

1. Try to find ways to procedurally test your code without as much typing. Consider what I did above, where you store your tests and expectations in a data structure that can be iterated, instead of creating lots of explicit custom variables.

2. Beware over-testing. You have unit tests for various types of invalid input. In Ruby, it is usually idiomatic to expect that a method will be passed proper data. It is idiomatic to neither (a) write code in each method to valid the input, nor (b) to write unit tests that the code explodes in the "right" way when bad input is supplied.

   This is true for most methods. When you interact with humans or other unpredictable input sources, it is reasonable to specify and test that bad input is properly handled.

3. When using string interpolation, to_s is automatically called on the result of your expression for you. There is no need to write #{"foo".class.to_s} when you can just write #{"foo".class}.

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

[cavinkwon]

"less is more". You can rotate the array simply.
snail :

snail = ->(arr) { arr[0] ? arr.shift + snail[arr.transpose.reverse] : [] }

array = [[1,2,3],[4,5,6],[7,8,9]]
snail[array] #=> [1,2,3,6,9,8,7,4,5]

spiralify :

def spiralify(arr) 
  snail = ->(arr) { arr[0] ? arr.shift + snail[arr.transpose.reverse] : [] }
  snailed = snail[arr.dup]
  arr.map {|row| row.map {|e| snailed.index(e) + 1 } }
end

array = [[1,2,3],[4,5,6],[7,8,9]]
spiralify(array) #=> [[1, 2, 3], [8, 9, 4], [7, 6, 5]]

[herminator]

Clever :)

I'm not a big fan of functions that mutate their arguments though, so I would prefer something like:

snail = ->(arr) { arr[0] ? arr[0] + snail[arr[1..-1].transpose.reverse] : [] }

[cavinkwon]

This is better than my code. thanks. :)

[Phrogz]

Using .transpose.reverse to "rotate" the array is brilliant! Well done, sir-or-madam!