Why is PolymorphicFinder so complex?

Ruby on Rails
1

I stumbled on this blog post and i’m trying to wrap my head around why PolymorphicFinder is so complex. Six patterns and recursion for what appears to be a simple problem.

2

@amccloud, I think the virtue of this implementation is that if further finders get added in the future, they are very easy to implement and add to the chain without having to create a huge messy if-else or case-when control flow.

I’ve used this pattern twice now for scenarios where finders are looking for similar records, and fall back to different and/or looser criteria (in some cases falling back over a dozen times) before getting to an acceptable set of results. We’ve changed our filter criteria several times, and it’s been fairly straightforward to swap strategies, reorder strategies, and to test the various components in isolation to have confidence in otherwise complicated logic.

3

Two things:

  1. You might want to watch the Weekly Iteration episode where we discussed this (linked at the bottom of that post) for an in-depth discussion of the code.

  2. Other (smart) people thought this was too much complexity for the payoff. You can happily hold that viewpoint yourself too!

4

It doesn’t look like that episode is directly linked from the blog post, so in case anyone else is looking for it, here it is: Polymorphic Finder.

5

There is also more discussion about the Weekly Iteration episode at this forum discussion:

http://forum.thoughtbot.com/t/questions-comments-for-weekly-iteration-4-polymorphicfinder-with-joe/1678

6

The primary reason for that class was to fix this issue, mentioned in the blog post:

Common problems, such as raising exceptions for bad IDs, could not be implemented in a generic fashion.

I think that blog post may not emphasize that particular issue enough; I decided to change the way this worked, because we kept reintroducing similar bugs as we modified the requested_purchaseable method. I introduced this class while fixing the third similar bug.

The new implementation makes it so that, when you add a new purchaseable type or ID parameter, you can’t forget to do things like raise if the record isn’t found.

Six patterns and recursion for what appears to be a simple problem.

I find that patterns add information to code, because they mean something more than a random class would. I also find that recursion is frequently the simplest, clearest solution to a problem, but I know that many people don’t work that way.

I also don’t think the problem is as simple as it seems. The original method hides a lot of complexity; if it were very simple, we wouldn’t have introduced so many bugs while changing it.

You could definitely implement a safe and generic version without using design patterns, classes, or recursion. If anybody wants to give it a shot, I’d be happy to take a look. Any solution should:

  • Look for one of many possible ID parameters.
  • If any of those ID parameters is present, look for a certain type of record based on a certain field.
  • If the ID is present but the record isn’t found, raise an exception.
  • The solution should not make it possible to add new parameter names, field names, or record types and forget to properly handle errors. For example, this alternate solution makes it easy to reintroduce the bugs I was trying to fix with new record types.
  • Bonus points for being well-tested.

If you’d like, you can submit it as a pull request to the Learn repository. The code in question is still in use.