geocoder icon indicating copy to clipboard operation
geocoder copied to clipboard

Set both a default radius and a dynamic radius column in near query

Open mnort9 opened this issue 2 years ago • 6 comments

When using the near query and specifying a dynamic radius column, is it possible to also set a default radius if the dynamic radius is not set for that row?

Something like: Location.near([lat, lng], 25, :effective_radius)

This would be more convenient than setting a default value in the database that might change later.

mnort9 avatar Dec 14 '22 16:12 mnort9

I don't see a way to do this, so I'm working on a PR.

Here's a couple options for the near arguments:

Location.near([lat, lng], :effective_radius, default_radius: 25)
Location.near([lat, lng], :effective_radius, fallback_radius: 25)

Optimally I think :effective_radius should be an option instead of an argument. It's a little wonky that the second argument can be either an integer or a symbol. This would flow a little better with the existing radius integer argument. So something like:

 Location.near([lat, lng], 25, radius_field: :effective_radius)

I don't think it'd be a breaking change if you still allow the db column as the second argument, but it might be a little confusing to allow both options.

mnort9 avatar Dec 15 '22 22:12 mnort9

On second thought, the radius_field option would then require the second integer radius argument. So maybe the first default_radius option is better.

mnort9 avatar Dec 15 '22 22:12 mnort9

Hey @mnort9, and thanks for this. Can you tell me a little more about when and how you would use this feature? I'm a little confused about the use case for it.

alexreisner avatar Dec 16 '22 16:12 alexreisner

@alexreisner Sure. An example use case would be assigning leads to sales agents that have different geographic territories. Agents in rural areas might need a larger radius, so a dynamic radius is stored in the db.

When a lead needs to be assigned to an agent, we'd want a query like:

Find an agent who's territory this lead is in based on the agent's coordinates and (set radius OR the default radius of X miles).

The alternative would be to set the default radius for every sales agent in the db, but then if it ever needs to be changed, it needs to be backfilled. It'd be easier to simply set the fallback radius in the db query and can just adjust the query default.

mnort9 avatar Dec 16 '22 19:12 mnort9

@mnort9 sorry for the delay here. I've been thinking about this. Thanks for explaining the use case--it makes sense. I think I can merge, but just a couple things first:

  1. What do you think about naming the option :radius_column_default? I think it needs to be more specific than "default" as it applies only to a very specific usage of the method.
  2. Instead of using present?, can you test for a value using methods available outside of Rails/ActiveSupport?

alexreisner avatar Feb 27 '23 03:02 alexreisner

@alexreisner I think in this case :radius_column_default is fine for the keyword argument. I agree that being explicit is important since there are multiple contexts of "default".

Aside from this though, I think it may be worth considering moving :effective_radius to a keyword argument. The fact that the second argument could be either an integer or a symbol makes it a bit more cumbersome to add on functionality to the method.

This is a good example that we're having to use keyword arguments like :radius_column_default to get around it. I agree with specific naming in this case, but I think the flaw is in method arguments design.

An alternative to consider:

Location.near([lat, lng], 25) # Normal usage
Location.near([lat, lng], radius_field: :effective_radius) # With radius column
Location.near([lat, lng], 25, radius_field: :effective_radius) # With radius column and column default

This could be backwards compatible to still allow existing implementations to use the second argument as the radius column. Thoughts?

mnort9 avatar Mar 01 '23 14:03 mnort9