0
\$\begingroup\$

I'm a beginner working through an exercise on ruby arrays. Given a two-dimensional array with 0's and/or 1's, it should return an array with the non-diagonal surrounding elements of 1 changed to 1 if they are 0. That is

a = [[1,0,0,0],
     [0,0,0,0],
     [0,0,0,0],
     [0,0,1,0]]

should return

[[1,1,0,0],
 [1,0,0,0],
 [0,0,1,0],
 [0,1,1,1]]

This is what I've come up with. Improvements or comments will be well received.

a = [[1,0,0,0],
     [0,0,0,0],
     [0,0,0,0],
     [0,0,1,0]]

#create a nil-filled placeholder copy of the array
new = Array.new(a.length).map! {|element| element = Array.new(a[0].length)}

#scan original array for 1; map crosses into new array if found
a.each_with_index do |m, n| # n == index of main array
    m.each_with_index do |x, y| # y == index of subarray
        if x == 1 
            new[n][y] = 1
            new[n][y+1] = 1 unless (a[n][y+1]).nil? #right
            new[n][y-1] = 1 unless (a[n][y-1]).nil? || y == 0 #left
            new[n-1][y] = 1 unless (a[n-1][y]).nil? || n == 0 #top
            unless n == (a.length - 1)
                new[n+1][y] = 1 unless (a[n+1][y]).nil? || n == (a.size - 1) #bottom
            end
        end
    end
end

#change nil's into 0's 
new.each do |element|
  element.map! {|x| x == nil ? x = 0 : x = 1 }
end
\$\endgroup\$
3
\$\begingroup\$

Copying the array

You are over-complicating the array copy. Array.clone will copy the objects of the array (but not the object's objects). Using it on array of objects might be problematical, but an array of ints will be fine.

new = a.map(&:clone)  # clone each row, return as an array

This line also removes the need of cleaning up your nils at the end

Additional Thoughts

While the logic is largely fine, stylistically your loops could be a little more idiomatic.

Unless you are using older ruby, you should tend to prefer .each.with_index over each_with_index. They do the same thing but .with_index is a little more functional (programming).

Secondly, in general, try to chose better variable names than m, n, x, y. You have comments describing what they are, but with better variable names, the code becomes self documenting. (I've used ix_row and ix_col, but that's my personal naming convention for indexes; feel free to change it, just make sure it's purpose is obvious.)

Finally, you are overthinking your boundary checks as well. All you need to do is simply check the index.

new.each.with_index do |row, ix_row|
  row.each.with_index do |cell, ix_col|
    if cell == 1
      new[ix_row][ix_col+1] = 1 unless ix_col+1 >= row.length #copy right
      new[ix_row+1][ix_col] = 1 unless ix_row+1 >= new.length # copy down
      new[ix_row][ix_col-1] = 1 unless ix_col.zero? # copy left
      new[ix_row-1][ix_col] = 1 unless ix_row.zero? #copy up
    end
  end
end
\$\endgroup\$

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.