Checking whether two strokes overlap [closed]

Question

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.

Answer 1

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!

Answer 2

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 

Answer 3

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