Matrix class with lots of tiny methods

I have been following the advice to make tiny methods that does just one thing and does it well. I also have been keen on reducing or eliminating duplication as much as possible.

But when a very experienced developer friend reviewed my code, he mentioned I was taking it too far. And, in short, my code was unreadable as it jumped too much killing the flow. Though I too feel I am pretty bad in naming my methods, I feel such tiny methods are helping me break down the problem and letting me solve it easily.

Challenge: Matrix Rotation (Move each matrix element to the neighboring position along concentric rectangular paths. The dimensions of the matrix are guaranteed to be even.)

Solution: matrix_rotator.rb

```
class Matrix
def initialize(two_d_array: nil)
@data = two_d_array
end

def rotate!(anti_clockwise: 0)
rotated = layers
.map { |layer| layer.map { |row, column| @data[row][column] } }
.map { |layer| layer.rotate(anti_clockwise) }

coordinates.zip(rotated.flatten).each do |(row, column), n|
@data[row][column] = n
end

self
end

def to_s
@data
.map { |row| row.join(' ') }
.join("\n")
end

def coordinates
layers.each_with_object([]) { |layer, a| a.push(*layer) }
end

def height
@height ||= @data.length
end

def width
@width ||= @data.first.length
end

def layers
number_of_layers.times.map do |index|
top(index) + right(index) + bottom(index) + left(index)
end
end

def horizontal_segment(index)
(width - (2 * index) - 1).times
end

def vertical_segment(index)
(height - (2 * index) - 1).times
end

def top(index)
horizontal_segment(index)
.map { |w| [index, w + index] }
end

def right(index)
vertical_segment(index)
.map { |h| [h + index, width - index - 1] }
end

def bottom(index)
horizontal_segment(index)
.map { |w| [height - index - 1, width - index - w - 1] }
end

def left(index)
vertical_segment(i

The way you dissected the problem was a good way to get a handle on it but once you had, it reveals some refactoring opportunities. Once you have the individual methods, you can begin to see how to build them back together

Let's begin by focusing on your two Matrix#_segment methods. Both have the same method signature and differ only by the use of width and height. So those could be combined to just be

def segment(index, length)   # maybe name it segment_enumerator just to be clear?
  (length - (2 * index) - 1).times
end

For top(index) then, you get

def top(index)
  segment(index, width)
    .map { |w| [index, w + index] }
end

which has the advantage of keeping width in the same function, making it slightly more readable and DRY.

Your side functions (top, right, et al.) really only differ by the block they use. So you could write a single map_side method that just passes a block in the map. However, because of the refactor we just did in the new #segment, a lambda helps pull it together:

def map_side(index, length, &block)
  lambda{ segment(index, length).map &block }
end

You might want to name that map_side something that means more to you in the problem domain (I know it returns an array of arrays, but didn't come up with a better name than map_side).

With that, we just use the new method to set up a quick hash of lambdas for each side, passing each the specific block you wanted:

def sides(index)
  _sides = {}
  _sides[:top] =    map_side(index, width)  { |w| [index, w + index] }
  _sides[:bottom] = map_side(index, width)  { |w| [height - index - 1, width - index - w - 1] }
  _sides[:left] =   map_side(index, height) { |h| [height - 1 - index - h, index] }
  _sides[:right] =  map_side(index, height) { |h| [h + index, width - index - 1] }
  _sides
end

Note, this is a hash of lambdas now, not the arrays you're looking for yet. But with it, we can now write this for layers in whatever order you need:

def layers
  number_of_layers.times.map do |index|
    _sides = sides(index)
    [:top, :right, :bottom, :left].inject([]) do |array, side| 
      array += _sides[side].call
    end
  end
end

A key thing to look at when you have tiny methods is whether you ended up using those methods more than once. I like labeling things too to keep them straight (and that's always a Good Thing), but when it's just a single use, a variable name is as good as a method name. So, we can just get rid of number_of_layers as a method. I called it count below, but whatever works for you.

def layers
  count = [height, width].min / 2
  count.times.map do |index|
    _sides = sides(index)
    [:top, :right, :bottom, :left].inject([]) do |array, side| 
      array += _sides[side].call
    end
  end
end

Now the following methods can be safely removed: horizontal_segment, vertical_segment, top, bottom, left, right, and number_of_layers.

The code is now DRYer, the width and height references are near each other for their specific uses, and you can quickly see how they stack up (in other words, your naming of elements was retained). The one part that might throw you if you're not used to it is how I used the lambda closures to pull this off, but it's not an uncommon approach in Ruby and as you can see, a handy one.

I tested this refactor with your test suite and it ran fine. I didn't benchmark, but it certainly didn't seem any slower and might be a little faster.

StackExchange Code Review Q#96597, answer score: 2