Take the 2-minute tour ×

I had a question regarding identifying all the points next to a given cell or set of cells) in a matrix (see Need a Ruby way to determine the elements of a matrix "touching" another element). Since no suitable ideas were put forth, I decided to proceed via brute force.

The code below successfully does what I sought to do. The array tmpl (template) contains a map of how to get from a given coordinate (provided by atlantis) to the 8 cells surrounding it. I then construct an array sl (shoreline) that contains all the “underwater” land touching the shoreline of atlantis by summing each element of atlantis with all elements of tmpl.

# create method to determine elements contiguous to atlantis
require 'matrix'            
atlantis = [[2,3],[3,4]]
tmpl = [[-1,-1],[-1,0],[-1,1],[0,-1],[0,1],[1,-1],[1,0],[1,1]]

ln = 0
sl = []
while ln < atlantis.length
  n = 0
  tsl = []
  while n < 8
    tsl[n] = [atlantis[ln], tmpl[n]].transpose.map { |x| x.reduce(:+) }
    n = n+ 1
  sl = sl + tsl
  ln = ln + 1
sl = sl - atlantis
sl.to_a.each { |r| puts r.inspect }

But I have a problem (one of many remaining) in that I still need 2 levels of loops above what’s shown here (one to keep adding land to atlantis until it reaches a set size and another to make additional islands, Bermuda, Catalina, etc.) and already this is becoming difficult to read and follow. My vague understanding of object oriented programming suggests that this cold be improved by turning some of these loops into methods. However, I learned to program 35 years ago in basic and am struggling to learn Ruby as it is. So my requests are:

  1. Is in fact better to turn these into methods?

  2. If so, would anyone be willing to show me how that’s done by changing something into an method?

  3. What do you do when you add additional levels and discover you need to change something in a lower method as a result? (e.g, after figuring out the simple case of how to create sl with just one value in atlantis, I had to go back and rework it for longer values.)

I hoping by asking the question in this way, it becomes something also useful to other nubies.

BTW, this bit .transpose.map { |x| x.reduce(:+) } I found on Stack Overflow (after hours of trying to do it ‘cause it should be simple and if I couldn’t do it I must be missing something obvious. Yeah, I bet you know too.) lets you add two arrays element by element and I have no idea how it works.)

share|improve this question
Maybe focus on understanding what you've written first. I'd also consider using a bit more whitespace so the code is easier to read. –  Dave Newton Jul 29 '12 at 23:54
One way to make your code more 'ruby-like' would be to replace the two while loops you have here with something more meaningful. Both could be replaced with the 'each' method (see ruby-doc.org/core-1.9.3/Array.html#method-i-each) which would make it a lot easier to understand what your code's trying to do. –  Pete Schlette Jul 30 '12 at 1:18
You only really need to make a method when you expect to be doing the same operation more than once. If this script only effectively calls this "method" once, there's no need to really change it. You could even argue that to do so would be premature optimization. –  lyonsinbeta Jul 30 '12 at 3:05
add comment

1 Answer

up vote 0 down vote accepted

already this is becoming difficult to read and follow

One way of making it less difficult to read and follow is to try to make the code "self document", by using readable variable names and Ruby idioms to reduce the clutter.

A quick refactor of your code gives this:

require 'matrix'            
atlantis = [[2,3],[3,4]]

shoreline = []
atlantis.each do |atlantum|
  shoreline += template.inject([]) do |memo, element|
    memo << [atlantum, element].transpose.map { |x| x.reduce(:+) }

shoreline = shoreline - atlantis
shoreline.each { |r| puts r.inspect }

The main processing block is half the size, and (hopefully) more readable, and from here you can use the extract method refactor to tidy it further if you still need/want to.

share|improve this answer
Thanks Pavling. Clearly more ruby like. Using each with atlantis is clearly the way to go. I'll need to learn inject as it is unfamiliar. Did you do this rewrite yourself or did you use a particular refactor(thanks too for tip to it)? If so, which one? –  user918069 Jul 30 '12 at 18:08
Yes, I just removed the intermediate variables and replaced the 'whiles'. –  Pavling Jul 30 '12 at 23:02
add comment

Your Answer


