3
\$\begingroup\$

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(index)
      .map { |h| [height - 1 - index - h, index] }
  end

  def number_of_layers
    [height, width].min / 2
  end
end

class MatrixRotator
  def initialize(source: nil)
    parse(source.readlines)
  end

  def result
    Matrix
      .new(two_d_array: two_d_array)
      .rotate!(anti_clockwise: @shift)
      .to_s
  end

  def parse(lines)
    parameters, *@content = lines.map(&:strip)
    @shift = parameters.split.map(&:to_i).last
  end

  def two_d_array
    @content.map { |line| line.split.map(&:to_i) }
  end
end

puts MatrixRotator.new(source: STDIN).result
\$\endgroup\$
2
\$\begingroup\$

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#<xxx>_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.

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for your time! I am so happy to see your answer! Made me thing well. As you rightly guessed, the use of lambda did get me confused; its not about how to use it. But it in the reasoning for picking lambda for the specific task. \$\endgroup\$ – Jikku Jose Jul 13 '15 at 9:36
  • \$\begingroup\$ Tried to refactor with the changes you mentioned and has updated in the repo; one thing I chose to keep was the method number_of_layers because I feel its better to keep such calculations away with proper naming. Anyways, thanks a lot for your very constructive comments. I appreciate the time you took. \$\endgroup\$ – Jikku Jose Jul 13 '15 at 10:26
  • \$\begingroup\$ The reason I went with lambdas was that I wanted the length for the segment to be carried through to where you needed the final map block and still make the code readable. Since a lambda closure would retain state from the time of its creation, then it could just be set in #sides to make it easy to use in #layers. An alternative would be to call #segment from within #layers, but since you were avoiding putting too much logic into #layers (and I agree), the lambdas seemed the easiest way to move the varied map block logic to where it was needed. \$\endgroup\$ – rdnewman Jul 13 '15 at 17:59
  • \$\begingroup\$ Okay, frankly I still don't follow as much. But anyways, I wouldn't have thought of listing lambda's like you did. Anyways, I am quite happy with the final refactor I was able to do with your help! Thanks a lot. \$\endgroup\$ – Jikku Jose Jul 14 '15 at 2:50
0
\$\begingroup\$

That's taking things a bit too far.

I attacked the problem you pointed me to in this way:

def read_matrix (fd) :
    m, n, steps = [int(x) for x in fd.readline().split()]
    arr = [[int(x) for x in line.split()] for line in fd]
    return (m, n, steps, arr)

def rotate_matrix (m, n, steps, arr) :
    z = (0,)*n
    out = [list(z) for x in range(m)]
    for layer in range(min(m, n) // 2) :
        rotate_layer (out, layer, m, n, steps, arr)
    return out

def rotate_layer (out, layer, m, n, steps, arr) :
    inds = clockwise_indices(layer, m, n)
    ninds = len(inds)
    brk = steps % ninds
    for (ix,iy),(jx,jy) in zip(inds, inds[brk:]+inds[:brk]) :
        out[ix][iy] = arr[jx][jy]

def clockwise_indices (layer, m, n) :
    cols = range(layer,n-layer)
    rows = range(layer+1,m-layer-1)
    return tuple((layer,col)         for col in cols) + \
           tuple((row,n-layer-1)     for row in rows) + \
           tuple((m-layer-1,n-col-1) for col in cols) + \
           tuple((m-row-1,layer)     for row in rows)


import sys
for row in rotate_matrix(*read_matrix(sys.stdin)) :
    print(' '.join((str(x) for x in row)))

Four functions, and a very simple script below the four functions.


In fact, those four functions could be rolled up into one function that still satisfies the single responsibility guideline (note the name: the function does exactly what the name says, and nothing more), and it isn't all that large:

def solve_matrix_rotation_challenge (fd) :
    m, n, steps = [int(x) for x in fd.readline().split()]
    arr = [[int(x) for x in line.split()] for line in fd] 

    z = (0,)*n
    out = [list(z) for x in range(m)]

    for layer in range(min(m, n) // 2) :
        cols = range(layer,n-layer)
        rows = range(layer+1,m-layer-1)
        inds = tuple((layer,col)         for col in cols) + \ 
               tuple((row,n-layer-1)     for row in rows) + \ 
               tuple((m-layer-1,n-col-1) for col in cols) + \ 
               tuple((m-row-1,layer)     for row in rows)
        ninds = len(inds)

        brk = steps % ninds
        for (ix,iy), (jx,jy) in zip(inds, inds[brk:]+inds[:brk]) :
            out[ix][iy] = arr[jx][jy]

    for row in out :
        print(' '.join((str(x) for x in row)))


import sys
solve_matrix_rotation_challenge (sys.stdin)


A key problem with a monolithic construction such as my solve_matrix_rotation_challenge is that it's hard to see what's going on without lots of commentary. A key problem with a slew of overly-short functions such as in your implementation is that it's hard to see how things fit together without lots of commentary. There's a happy medium where you can still understand individual chunks of code and where you can see how things fit together, and without needing an excessive amount of commentary.

Some comments about my code:

  • I explicitly chose to use a separate output array rather than rotating in place. The challenge said the array sizes would be rather small. If the challenge said that the arrays might be up to 30000*30000, I would have rethought this. It instead said 300*300 was the max size. In-place mucking around oftentimes involves convoluted logic. I chose to keep it simple.

  • Sites such as the one you referenced care about speed of execution. There's some hackery in my code specifically aimed at speed (e.g, z=(0,)*n; out=[list(x) for x in range(m)]). Aiming at speed is usually undesirable. Without that need for speed, I would have just used out=copy.deepcopy(arr).

Some comments about your code:

  • Your functions are so very, very small that I can't see how they fit together as a cohesive whole.

  • Many of your one-liners are trivial. You may not have seen them as such at first, but once you do, think about eliminating them and expanding the calls into the implementations.

  • Can you handle a shift of 999999937?

\$\endgroup\$
  • \$\begingroup\$ Thanks, I didn't understand the last question? \$\endgroup\$ – Jikku Jose Jul 13 '15 at 9:40
0
\$\begingroup\$

The other answers are already very good, but I want to point out one stylistic detail the other answers missed: Matrix#rotate! should be called Matrix#rotate, there should be no bang in the method name.

The bang is used to point out the more surprising one of a pair of methods, i.e. when you have both rotate and rotate!, then rotate! is the more surprising one. However, you only have one method, so it shouldn't have a bang.

Bang methods always come in pairs!

\$\endgroup\$
  • \$\begingroup\$ Thanks didn't know it should come in pairs. I just used it since I was doing in place computation replacing the initialised variable. \$\endgroup\$ – Jikku Jose Aug 1 '15 at 9:51
  • \$\begingroup\$ As an example: Process::exit! has nothing to do with mutation, its "surprising" behavior is that it exits the process without running the exit handlers. OTOH Array#clear doesn't have a bang, despite mutating the receiver. \$\endgroup\$ – Jörg W Mittag Aug 1 '15 at 10:46
  • \$\begingroup\$ They are great examples. I was assuming mutation or a non idempotent operation is denoted by the bang operator. \$\endgroup\$ – Jikku Jose Aug 2 '15 at 4:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.