# Change non-diagonal surrounding elements in a 2-D array

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


### 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

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