ActiveRecord extension to find next / previous record(s) in one query
I will stick to reviewing the code you've posted, although it seems tied to other code.

First of all: Use descriptive variable names. x, y, q, and so forth aren't great for legibility. Use query, position and similar full words. Especially since you're dealing with complex nested arrays.

There also seems to be some logical issues/dangerous assumptions. I don't know if you check/sanitize your input elsewhere, but if you don't things will get weird. E.g.:

# This will fail if x[i] is nil
x_cond = [x[i][0].presence, x[i][1]]

q &lt;&lt; x_cond if x_cond[0]

if i &gt;= 1
  # 1. This *may* append ["AND"] etc. directly to an empty array,
  #    if x_cond[0] was nil above, resulting in a nonsense query.
  # 2. This will fail if y.count &lt; i - 1
  q &lt;&lt; ['AND'] &lt;&lt; y[i - 1]
end


In other words, if x_cond[0] isn't present, you don't append anything to the array. Yet the following if block assumes that something definitely was appended. And there's no check for the length of y so y[i - 1] may fail. And x[i][0].presence will fail if x[i] is nil to begin with.

I also see some (dangerous) redundancy, like the n = x.length method parameter. There's no reason for this to be a method parameter. In fact, it's detrimental, because the n value is never checked, so you can pass any value like n = -42 which wouldn't make sense.

Moreover, the code doesn't actually match its description: Your comment for build_query_factory states that the result should be something like:

x0 OR y0 AND (x1 OR y1 AND (x3 OR y3))


but the method returns:

x1 OR (x2 AND y1 OR (x3 AND y2))


Notice that a) the structure is very different, and b) y3 doesn't even appear.



Now, assuming that you want the result you give as an example in the comment (and not the result the method actually returns), I would do something like this:

def build_nested_query(x, y)
  # zip the two arrays (x, y, x, y, x, y, ...)
  ordered = x.zip(y)

  # split into conditions and parameters
  ordered = ordered.map do |x, y|
    [ [x.first, y.first], [x.second, y.second] ] # still confusing, to be honest
  end

  conditions = ordered.map(&amp;:first)
  parameters = ordered.map(&amp;:second).flatten # thanks to ActiveSupport for Array#second, by the way

  [ nest_conditions(conditions) , parameters ]
end

def nest_conditions(conditions)
  # create "x OR y" string
  branch = conditions.first
  branch = branch.join(" OR ")

  # nest the remaining recursively, appending them with " AND "
  remaining = conditions[1..-1]
  if remaining.any?
    nested = nest_conditions(remaining)
    branch += " AND (#{nested})"
  end

  branch
end


That'll do something like this:

x = [["x1", [:x1]], ["x2", [:x2]], ["x3", [:x3]]]
y = [["y1", [:y1]], ["y2", [:y2]], ["y3", [:y3]]]

query = build_nested_query(x, y)
query[0] #=&gt; "x1 OR y1 AND (x2 OR y2 AND (x3 OR y3))"
query[1] #=&gt; [:x1, :y1, :x2, :y2, :x3, :y3]


Which I believe is what you want. You'll want to do some sanity-checking/sanitizing of x and y beforehand, to make sure they make sense.

Of course, if you find a better way to supply the method parameters in the first place, the code can be cleaned up even more.

I would also consider encapsulating conditions and parameters somehow to avoid the non-descriptive [0] and [1] (or first/second) stuff in favor of something more descriptive (Rails no doubt already has an internal class for this structure. Otherwise, a simple Struct-based class should do.)



I haven't looked closely at where_relative, but again I see some strange logic:

def where_relative(spec, mode)
  ord = spec.order

  # I assume `values` is an instance variable?
  # Anyway, why assign `value` here? It's only used in
  # the else-block, and so should be assigned there
  value = values[spec.name]

  if ord.is_a?(Array)

    # ...

    # Wait, now you're assigning something to `values`?
    values = mode == :after ? ord.from(pos + 1) : ord.first(pos) if pos

    # and apparently, that something can be nil, since you're
    # checking for that here - but you didn't check for that
    # before you called `values[spec.name]` above
    return EMPTY_FILTER unless values.present?

    # ...

  else

    # ...

    # And here you use `value` (i.e. values[spec.name])
    ["#{spec.col_name_sql} #{op} ?", [value]]

  end
end


Now, I admit I don't know the exact sequence of events, but the above looks/smells like dangerous assumptions and nasty side effects. If the method is called several times per object instantiation, results get unpredictable. First time it's called, I assume values is set to something. But it might get set to nil by the method. This will cause the next call to the method to crash on values[spec.name]. If the method is so completely dependent on values, it should be a parameter.

Lastly, this line should really be reworked:

values = mode == :after ? ord.from(pos + 1) : ord.first(pos) if pos


Don't use a ternary and postfixed if on the same line - it's very confusing.