1
\$\begingroup\$

I have these two functions and I was just wondering if there is any way to shorten the line of code with the many && statements. Shortening of the rest of the code would be cool too, but it's not necessary. I'm trying to learn if there is a better way to accomplish this though.

# Accepts two stroks and determines if they overlap with one another
def overlap? stroke1, stroke2
    min_x = 0
    max_x = 1
    min_y = 2
    max_y = 3

    bounds1 = get_bounds stroke1
    bounds2 = get_bounds stroke2

    return true if (bounds1[max_x] >= bounds2[min_x]) && (bounds1[max_y] >= bounds2[min_y]) && (bounds2[max_x] >= bounds1[min_x]) && (bounds2[max_y] >= bounds1[min_y])

    return false
end 

# Returns an array of bounds [x_min, x_max, y_min, y_max] for the stroke
def get_bounds stroke
    # Arrays for storing x and y values for the stroke currently being inspected
    xvals = Array.new
    yvals = Array.new

    stroke.each_slice(3) do |point|
        xvals.push(point[0])
        yvals.push(point[1])
    end

    # A temporary array that stores the min/max x/y for the stroke being inspected
    bounds = Array.new
    bounds << xvals.min << xvals.max << yvals.min << yvals.max

    return bounds
end

I actually did not write this code, but I offered to clean it up and I came upon this and thought that there was probably a better way.

\$\endgroup\$

closed as off-topic by 200_success Jun 2 '15 at 17:29

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

1
\$\begingroup\$

The code in the if is duplicate, so we can extract it to a method that I will call gte (greater than or equal). The maxs and mins values ​​are used at this comparison, so they will be moved to the new method too:

def gte(bounds1, bounds2)
    min_x, max_x, min_y, max_y = 0, 1, 2, 3

    bounds1[max_x] >= bounds2[min_x] && bounds1[max_y] >= bounds2[min_y]
end

The content in the if expression changes to:

gte(bounds1, bounds2) && gte(bounds2, bounds1)

But the if is not necessary because this expression already returns a boolean value, then we will return it directly. The keyword return is not required, then the method overlap? code looks like this:

def overlap?(stroke1, stroke2)
    bounds1 = get_bounds stroke1
    bounds2 = get_bounds stroke2

    gte(bounds1, bounds2) && gte(bounds2, bounds1)
end

Maybe you have a better name than gte, but remember that it must have a meaningful name!

\$\endgroup\$
  • 1
    \$\begingroup\$ I would avoid using the and operator because it has different precedence than && and can behave unpredictably. \$\endgroup\$ – nickcoxdotme Jun 14 '13 at 4:55
  • \$\begingroup\$ Ah I thought about switching the bounds, but I didn't think about doing both bounds comparisons in one and then switching the bounds. \$\endgroup\$ – snowe2010 Jun 14 '13 at 5:23
  • \$\begingroup\$ You're right @nickcoxdotme, thanks for the correction! \$\endgroup\$ – Juliano Alves Jun 14 '13 at 13:35
0
\$\begingroup\$

I would echo @Juliano's answer. Also, when you're assigning variables like you are in the first method, it helps to use parallel assignment. It also helps to use parentheses for method arguments when using a predicate method. That is the style you see in the Rails source. Thus:

def overlap?(stroke1, stroke2)
    min_x, max_x, min_y, max_y = 0, 1, 2, 3

    ...
end 
\$\endgroup\$
0
\$\begingroup\$

Some notes:

  • Use 2 spaces for indentation.
  • Don't write explicit return.
  • You may omit parenthesis, but while that's (sometimes) idiomatic in calls, almost nobody does it in signatures (they are harder to read).
  • overlap? uses constants as indexes, note this is a low-level way (C and alike) of doing things. De-structure the value instead.
  • return true if condition + return false -> condition. In any case, try not to write with this imperative style, think in terms of expressions.
  • get_bounds is very convoluted, definitely not the way you do it in Ruby, with a bit of functional programming practice you'll see that all this method can be simply written stroke.transpose.map(&:minmax).

I'd write:

def overlap?(stroke1, stroke2)
  (min_x1, max_x1), (min_y1, max_y1) = get_bounds(stroke1)
  (min_x2, max_x2), (min_y2, max_y2) = get_bounds(stroke2)
  max_x1 >= min_x2 && max_y1 >= min_y2 && max_x2 >= min_x1 && max_y2 >= min_y1
end 

def get_bounds(stroke)
  stroke.transpose.map(&:minmax)
end
\$\endgroup\$
  • \$\begingroup\$ can you explain the stroke.transpose.map(&:minmax) line? I really don't understand what is happening there. \$\endgroup\$ – snowe2010 Jul 2 '13 at 0:19
  • \$\begingroup\$ @snowe2010: I'll try, but it's a lot easier if you create an example "stroke" in irb and run each part of the expression. "transpose" makes [[1, 2], [3, 4], [5, 6]] -> [[1, 3, 5], [2, 4, 6]] so now all coordinates of the same axis are together. Now, for each of those arrays, we want the minimum and maximum to get the bounding box, so we map with the "minmax" method (which returns pair [min, max]). That returns: [[1, 5], [2, 6]], which is the bounding box you wanted. I know it seems cryptic, but it's the standard mathematical way to do it, nothing fancy. \$\endgroup\$ – tokland Jul 2 '13 at 12:12

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