Separation of concerns dilemma in Rails

Question

I'm trying to make logging for my rails app and have some dilemmas over philosophy used in rails. My app has Link model which has_many Hits:

class Link < AR::Base
  has_many :hits
end

class Hit < AR::Base
  belongs_to :link
end

Now each time the link is being hit, I call hit! method to record a request on the link (to keep the controller skinny I make the model fat):

class LinksController < ApplicationController
  def hit
    link = Link.find(params[:id])
    link.hit!(request)
  end
end

class Link < AR::Base
  def hit!(request)
    params = extract_data_from_request(request)
    hits.create(params)
  end
end

Now here's where I'm confused. I want to record the data that came with request object (like remote ip, referrer, user agent, etc.) so I need to pass request object down to model, but I believe that this does not conform to "separation of concerns" and blurs responsibility lines in MVC design pattern (of course, correct me if I'm wrong). Also if I'll create a Hit object in controller itself, then I'm making skinny model and fat controller:

class LinksController < ApplicationController
  def hit
    hit_params = extract_data_from_request(request)
    Hit.create(hit_params.merge(:link_id => params[:id])
  end
end

Although the latter case makes testing much easier (I don't need to mock request in model specs) - it just doesn't seem right.

Any advice on this - much appreciated.

P.S. extract_data_from_request(req) method is placed in appropriate places where needed. It returns a hash of needed attributes for Hit object.

Answer 1

Personally I would be wary of over-thinking these things too much.

The concept of a hit is very much tied to a website or web application, as is the concept of a (HTTP) request. The fat controller anti-pattern is more about having lengthy controller actions that contain ActiveRecord find statements and business logic (often characterised by if/elsif/else blocks) that can easily be extracted into the model.

Controllers have certain orchestration responsibilities. Creating an object within one isn't a heinous crime. After all, we do it all the time in our create actions.

Answer 2

Yeah, i'd agree with John. The concept of a request would normally be a 'controller thing' but in this case your model is modelling a request, so it's definitely in model territory in this case. Effectively once the request object crosses the boundary from controller into model it's just another object, with no special properties: it's no longer concerned with the process of getting and responding to html requests, it's just an object that you can do whatever you want with.

One thing to be wary of, though, is that in ruby arguments are passed by reference. That means that the request object you are manipulating in your model is THE SAME OBJECT as the one being dealt with in the controller. I may be being overly paranoid (or just plain wrong) but you might want to pass a duplicate of it to the model rather than the actual request itself. ie

class LinksController < ApplicationController
  def hit
    link = Link.find(params[:id])
    link.hit!(request.dup)
  end
end