Polymorphic Finder, with Joe Ferris

Upcase
1
In this episode, Joe and Ben discuss a fairly advanced example of refactoring. You'll dive into the good, the bad, and the ugly of code before and after being rewritten. The new code uses a number of design patterns, including the Builder pattern...
This is a companion discussion topic for the original entry at https://thoughtbot.com/upcase/videos/polymorphic-finder-with-joe-ferris
2

You can watch the video here: https://learn.thoughtbot.com/shows/23-the-weekly-iteration (Episode 4)

Or read the post here: Code Show and Tell: PolymorphicFinder

Or just skip to the code here: https://bitly.com/polyfindersource

3

First off, great video mechanics on this one: your switching between showing the code and the “talking heads” was excellent!

Second, my gut-level response is that this is really neat code, but I’m not convinced that the added behind-the-scenes complexity is worth it. Maybe just my level of code grokkery, but there you have it.

I did like the explicit discussion of the code patterns; great learning session. Thanks.

4

I agree with @JESii. While I found the use of code patterns interesting I didn’t care for the end result. I don’t think the arguments to finding are very clear, and I suspect future developers will be totally lost trying to grok the implementation.

Having said that it was really interesting to see the patterns in a different perspective and along with code we are familiar with: AR queries, Rack, etc.

5

Great video! Loved the solution and how it solved the problem you were having.

I can see where @JESii and @noahhendrix are coming from but I would have to contend that a quick dive into the PolymorphicFinder class would result in most people being able to figure out what is going on “under the hood”.

In regards to the clarity of the arguments to finding, I guess you could make it a bit for semantic by doing something like:

finding(on: Class, with: :id, using: [:param])

That might make it a bit clearer but I’m not convinced that the value is there.

At any rate thanks @benorenstein and @jferris for another great video!

6

Thanks for your comments.

I agree that, on a gut level, this feels like a high amount of complexity for the problem it’s designed to solve. If it can be simpler, that would be great.

Here are the main things I was trying to fix in my refactoring:

  • The logic for finding an item from params was duplicated in each if block, which risked reintroducing the same bugs. The same bug (this method was returning nil in two cases instead of raising) had already occurred several times.
  • This risk was compounded by the fact that the exceptional behavior wasn’t covered by tests, so I wanted a solution that was easy to test.

I’d love to see somebody else’s attempts at simplifying this solution while achieving the goals listed above. You could fork my gist (https://gist.github.com/jferris/2ed8ecab1ff068a5be3e) or fork the whole Learn repository (https://github.com/thoughtbot/learn) if you’re interested in taking a crack at it.

7

One of the things I appreciated about the Weekly Iteration episode was the open admission that the increased complexity was there, but it was worth the tradeoff. I love static analysis, and am a frequent user of things like Cane, Flog, Reek, Rails Best Practices, etc., but I think it’s easy to get into situations where sometimes in a zeal for low Flog scores, etc. you can make your code worse.

I have a situation with a schedule generator for a sports league. The method that provides the matrix of team pairings each week is just a long hash of numbers. Refactoring that into a method that’s “better” just because it has less complexity makes the code harder to read and more difficult to understand. Complexity should be a guide. If something’s complex, then you should have to have a very good reason for it to be that way.

8

So even though the new implementation leaves a bad taste in some stomaches, the one pro of the new PolymorphicFinder class being easy to test seems worth the change. Personally I’d prefer not to test any controller code so it needs to be simple, and complexity should be abstracted.

I’m glad you guys pulled this out for discussion. Being able to talk through each design pattern and then see them all working together in a single class is fantastic.

As to the issue of hitting this code as a new developer, it seems the finding method is easy enough to infer what I would need to do to add a new purchasable. If I dove into the class and the design patterns used were confusing, it would be a great opportunity to get an introduction to these concepts.

The original code feels like something I would add to and keep reintroducing the same bugs. This refactor, in my mind, is defiantly an improvement.

9

I loved this episode and the blog post for the same reasons as others: seeing patterns used and defined.

The PolymorphicFinder seems like great library code but I think I would prefer different code inside an application for the same reason as @noahhendrix:

I don’t think the arguments to finding are very clear

When @benorenstein said in the episode that the code in ApplicationController was clearer, I wasn’t sure I agreed:

def requested_purchaseable
  PolymorphicFinder.
    finding(Section, :id, [:section_id]).
    finding(TeamPlan, :sku, [:team_plan_id]).
    finding(IndividualPlan, :sku, [:individual_plan_id]).
    finding(Product, :id, [:product_id, :screencast_id, :book_id, :show_id]).
    find(params)
end

The relationship between the array of attributes and the database columns isn’t clear to me. The relationship between params and its keys are also pulled apart and lost.

The things I love about the refactoring that seem important to keep:

  • Avoiding the ease of re-creating the bug caused by a nil response in the where(...).first methods.
  • Moving code out of the ApplicationController junk drawer to improve testability.

I played around with an alternative implementation and came up with this:

class Purchaseable
  def initialize(params)
    @product_id = params[:product_id] ||
      params[:screencast_id] ||
      params[:book_id] ||
      params[:show_id]
    @individual_plan_id = params[:individual_plan_id]
    @team_plan_id = params[:team_plan_id]
    @section_id = params[:section_id]
  end

  def self.find(params)
    new(params).find || raise "Couldn't find purchaseable object from #{params}"
  end

  def find
    product || individual_plan || team_plan || section
  end

  private

  def product
    if @product_id
      Product.where(id: @product_id).first!
    end
  end

  def individual_plan
    if @individual_plan_id
      IndividualPlan.where(sku: @individual_plan_id).first!
    end
  end

  def team_plan
    if @team_plan_id
      TeamPlan.where(sku: @team_plan_id).first!
    end
  end

  def section
    if @section_id
      Section.where(id: @section_id).first!
    end
  end
end

Wherever requested_purchaseable was used, we would directly use Purchaseable.find(params). It maintains the ability to add new types without accidentally re-introducing the nil-caused bug.

In addition to solving the two bullet points I listed above, I like this implementation because it uses domain-specific vocabulary (Purchaseable) instead of general programming/implementation terms (PolymorphicFinder): a Purchaseable in this system is a product or individual_plan or team_plan or `section.

It includes some repetition and conditionals, but I prefer that tradeoff because I think the code is readable, helps the team understand the data structures in the domain, and adding, editing, or removing types is still obvious and safe.

I don’t know the likelihood of this hypothetical change, but if the logic for finding an individual type changes, I believe PolymorphicFinder would also be more resistant to that change than this implementation.

10

Looks like the video isn’t loading. You can download the video, but the player won’t load in the browser for me.

11

@DavidVII which browser are you using? Just asking since I’ve noticed a similar issue in Firefox 39.0 on OS X, it’ll play through part of the video and then the video will stop, but the audio will continue. I ‘think’ this may be a bug in Firefox though since playing the same videos (like this one WeeklyIteration episode) does work in Firefox Developer Edition as of today: 8/22/2015.

12

Hi, this episode is very inspiring. Knowing anti-pattern would really help to build neat, simple and reusable code.
I am wondering is there any book introducing anti-pattern, such as telescoping constructor anti-pattern? Thanks in advance. Any suggestion is very appreciated!