Controller refactoring question

[RocketR]

Hello everyone.

I'm struggling with a design dilemma and want to share my mindflow in case someone can help me out. Here's the important code:

    Routes
      constraint Subdomain do
        get 'things/:slug' => 'things#show
      end

      resources :things

    ThingsController#show
      scope = if request.subdomain.present?
        owner = Owner.find_by_subdomain(request.subdomain)
        owner.things.where(slug: params.fetch(:slug))
      else
        Thing.where(id: params.fetch(:id))
      end

      @thing = scope.available.first!
      respond_to do |format|
        ... some code here...
      end

    rescue ActiveRecord::RecordNotFound

      @thing = scope.first!
      respond_to do |format|
        ... some other code here...
      end
    end

The code doesn't look too awful, but there's a problem with testing - there's too much combinations to test and it hits the DB, so it's very slow. If I start stubbing things the test will become fragile and unclear, let alone that stubbing `where(...)` is bad. So my next step would be to extract this logic into a separate object. This is purely a controller concern, so I'm not going to put it in a model, lib etc. How about a local object ShowScope:

    ThingsController::ShowScope
      def self.build(params)
        if params[:subdomain].present?
          owner = Owner.find_by_subdomain(params[:subdomain])
          owner.things.where(slug: params.fetch(:slug))
        else
          Thing.where(id: params.fetch(:id))
        end
      end

This doesn't help me too much, as to test the `if` in the ShowScope I still have to hit the DB and still have to do some excessive stubbing for the `.available.first!` call. Extract more objects?

    ThingsController::ShowScope
      class ByOwnerAndSlug < Struct.new(:owner, :slug)
        def call(scope)
          owner.things.where(slug: slug).first!
        end
      end

      class ById < Struct.new(:id)
        def call(scope)
          scope.where(id: id).first!
        end
      end

      def self.build(params)
        if params[:subdomain].present?
          owner = Owner.find_by_subdomain(params[:subdomain])
          ByOwnerAndSlug.new(owner, params.fetch(:slug))
        else
          ById.new(params.fetch(:id))
        end
      end

    ThingsController#show
      query = ShowScope.build(params.merge(subdomain: request.subdomain))

      @thing = query.call(Thing.available)
      respond_to...

    rescue ActiveRecord::RecordNotFound

      @thing = query.call(Thing.scoped)
      respond_to ...
    end

Now I can test it, but hey, I had 5 lines of code, now I have 3 extra objects, isn't this over-engineering?

Let's step back and try another way. In the router we have a context that is lost in the controller. What if we just attach each route to a different controller:

    Routes
      constraint Subdomain do
        get 'things/:slug' => 'owner_things#show
      end

      resources :things

    OwnerThingsController#show
      owner = Owner.find_by_subdomain(request.subdomain)
      scope = owner.things.where(slug: params.fetch(:slug))
      @thing = scope.available.first!
      ...

    ThingsController#show
      scope = Thing.where(id: params.fetch(:id))
      @thing = scope.available.first!
      ...

We avoided one `if` so the code is more straightforward, but we still need to encapsulate this AR-stuff somewhere and the `respond_to ...` part now has to be duplicated. Maybe we could fix this with controller inheritance but again it will just make things complicated and less flexible.

What do you think? I see this pattern quite often in show/index actions and would be happy to solve it once and for all.

[Andrew Premdas]

--
You received this message because you are subscribed to the Google Groups "Objects on Rails" group.
To unsubscribe from this group and stop receiving emails from it, send an email to objects-on-rai...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

This might be slightly nuts, but why not treat the top domain as a subdomain, thus removing the conditional logic. Sort of like applying the NullObject pattern

Andrew

--

------------------------

Andrew Premdas

blog.andrew.premdas.org

[Roman Kushnir]

The branches differ not only by presence of a subdomain. And in fact, in my first approach I'm using something similar - I wrap AR-calls in objects to unify the interface.

2013/7/19 Andrew Premdas <apre...@gmail.com>

--
You received this message because you are subscribed to a topic in the Google Groups "Objects on Rails" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/objects-on-rails/cx74Ersj_P0/unsubscribe.
To unsubscribe from this group and all its topics, send an email to objects-on-rai...@googlegroups.com.

[Paul Hamann]

"So my next step would be to extract this logic into a separate object. This is purely a controller concern, so I'm not going to put it in a model, lib etc. "

I think you're asking some good questions.  I agree that controllers should have limited knowledge of AR.  For me, an AR model is a thin wrapper around persistence.  It's responsibility is to know how to access AR, so that other classes do not have to know.  The best way to do that is to wrap the queries with an intention-revealing message on the model.  That also gives you a good point to stub, and makes it easier to set message expectations.  I think that you're gut is correct that the classes that you abstracted may not be worth the added indirection and complexity.

This first message that I would add to the class may seem minor.  But consider Rails 4.
owner = Owner.by_subdomain(request.subdomain)
Otherwise, Rails 4 would break the original with it's new syntax.
owner = Owner.find_by_subdomain(sub) becomes Owner.find_by(subdomain: sub)
It's better to isolate that change in one place.

Similarly, you could have an instance method that wraps this query:

scope = owner.things.where(slug: params.fetch(:slug))

becomes
scope = owner.things_by_slug(params.fetch(:slug))
or maybe
scope = owner.available_things_by_slug(params.fetch(:slug))

Even this seemingly trivial access of AR in the controller can be wrapped.
Thing.where(id: params.fetch(:id))
becomes
Thing.retrieve(params.fetch(:id))

While it may seem like a small win, it wasn't that long ago that we didn't have a :where method, only find. Again, it makes sense to isolate knowledge of AR in one place while naming messages to reveal your intentions.

As for eliminating the if, here's the principle that I follow:  Make the decision at the earliest point possible. In this case, that would be your route file.

HTH.
--Paul

[RocketR]

Thanks Paul,
I've stumbled into a similar situation again and came back here to check. I guess, I'll wrap it in an AR-method as you recommend.