Topic: Factoring CRUD functionality out of a controller

I'm trying to work out how to make my controller skinnier. I've read this great article http://therailsway.com/2007/6/1/railsco … ontrollers which sheds some light on how to delegate some code to a from a Controller to a Model for a class whose responsibility to provide information. This example is more a case of illustrating that you don't need db tables to be behind a Model to make it justify creating a model and thus does not suit my situation...

I have a controller which is littered with private methods to help save data for two different models which depend on each other. Here's a simplified example

* I have a CustomerController and behind this I have CustomerModel and a ClubModel
* A Customer can be associated with zero or many Clubs
* If a Customer leaves a Club I delete a join table record in CustomerClubs. Then I check if any other Customers are associated with this Club. If not then I delete the Club record as well for house keeping.

Note: the starting point for this scenario is the body of the after_update_save method. Please be aware that I've just typed this example up on the fly so it may (or almost certainly!) contains typos.

***************************************************************
CustomerController < ApplicationController

  def after_update_save(record)
    # Starting point: On reaching here the the Customer has saved,
    # let's say they used to be originally be members of 'The Happy Rats' club
    # and the 'The Wary Mice' club now they are only members of 'The Happy Rats'
    # club. We must reflect them as leaving 'The Wary Mice' by updating the db
    # appropriately
    @record_under_update = record # SEE QUESTION 1
    update_clubs
  end

private
  def update_clubs
    join_club
    # This will create a new Club record if this Club does not already exist
    # and associate it with the customer, SEE QUESTION 2
    leave_club
    # This will disassoc from the club (and delete the actual Club record if
    # no other customers are associated with it)
  end

  def join_club
    # TODO
  end

  def leave_club
    # WIP
    clubs_to_leave.each do |club_id|
      leave_club_and_delete_resulting_orphans club_id
    end
  end

  def clubs_to_leave
     original_club_id_list - final_club_id_list
     # Subtracting these gives an array of club ids to disassociate the customer from
  end

  def leave_club_and_delete_resulting_orphans(club_id)
    customer_club = CustomerClub.find_many_by_club_id_and_customer_tag_id(
      :customer_id => @record_under_update.object_id,
      :club_id => club_id).first
    CustomerClub.delete customer_club # call to model's static method
    delete_club_if_orphaned club # SEE QUESTION 3
  end

  def delete_club_if_orphaned(club)
    Club.delete(club.object_id) if orphaned_club?(club)
  end
 
  def orphaned_club?(club)
    other_usages = CustomerClub.find_many_by_club_id(
      :club_id => club.object_id)
    other_usages.blank? == true
  end
end
***************************************************************

My main question is, for unit testing I can only access 'after_update_save' as it's the only public method. So should I refactor the private methods into a separate class so that I can test them as public methods? If so how should I go about this? Also...

QUESTION 1: Is this bad practice? I've made a class instance var called @record_under_update and assigned it the record var to avoid needing to pass it around too much as a function arg. Is this ok?
QUESTION 2: Should I call this 'join_club_if_required' to make it's functionality clearer or is it a bit naff to have the word 'if' in method names? This also applies to the naming of 'leave_club'.
QUESTION 3: Again, is it bad to have the word 'if' in method names?

Happy hacking!
froggie

Re: Factoring CRUD functionality out of a controller

you should move all of this into the Model. why do you want to keep it in the controller after you read the article you link to above? any specific reasons?


Better yet, you should have called the jointable something like club_memberships and create a controller and model for it, and all this junk becomes a piece of cake with a few lines of code:

class Customer
  has_many :club_memberships
  has_many :clubs, :through => :club_memberships
end

class CustomerClub
  belongs_to :club
  belongs_to :customer
  before_destroy :check_for_orphaned_club


private
  def check_for_orphaned_club
    if self.club.club_memberships.count = 1
      self.club.destroy
    end
  end
end

class Club
  has_many :club_memberships
  has_many :customers, :through => :club_memberships
end

#ClubMembershipsController
def destroy
  @membership = Clubmembership.find(params[:id])
  @membership.destroy
end


Besides the association code, this is like what, 3-4 lines of code? And your controller is clean as a baby smile Gotta Love RESTful design ...

Last edited by Duplex (2008-02-04 14:23:07)

Re: Factoring CRUD functionality out of a controller

Thanks for that Duplex. I didn't realise that it was ok to refer to 'other' models from a given model (eg. club.club_memberships.count) as I thought that each model was to be responsible for itself only. I was thinking of the controller as an mediator between these models. But on reflection that makes about as much sense as non-alcoholic beer smile

Your helpful sample is much appreciated. Very elegant!

Kind regards,
ff

Last edited by frogface (2008-02-04 15:54:07)

Re: Factoring CRUD functionality out of a controller

you define your associations between models inside of the models themselves, so it makes perfect sense that you do the clean-up on delete etc. inside them too wink

A controller should really only do what it is asked by the users request (= the URL): create a record, delete a record, show the latest 10 records etc.pp. Everything else should go somehwere else, like this stuff above in the model, or sidebar code in partials and helpers instead before_filters and the like ... keep your controllers slim....

Everytime you find yourself coding methods with strange names in your controller, its probably time to refactor wink

Last edited by Duplex (2008-02-04 17:06:27)

Re: Factoring CRUD functionality out of a controller

Just wanted to add that there are some excellent plugins out there for making your controllers ultra-thin

make_resourceful

resource_controller

Re: Factoring CRUD functionality out of a controller

Duplex,

I've stumbled upon a problem when modelling the many-to-many relationshop as CRUD. The HTTP operation to destroy a membership in your example would be DELETE /memberships/1, where '1' is the id of a membership. This can be dangerous since we're exposing an interface where users can delete arbitrary memberships that are not necessarily theirs. There would be a few solutions:

1. Make sure the membership is actually associated to the logged user, but that would take an extra query to the database.

2. Let the :id of /memberships/:id be a Club id, not a Membership id... But changing the semantic of the id looks messy. Plus, I can't get the code to delete a membership any better than this, which also takes two queries (but this can just be me...):

def destroy
  @logged_user.memberships.delete(Club.find_by_id(:id))
end

I haven't been able to find an elegant way to work around this... What do you think?

Javier

Re: Factoring CRUD functionality out of a controller

At the top of the controller, put:

before_filter :make_sure_user_has_ownership, :only => :destroy

The destroy method:
def destroy
  @membership = ClubMembership.find(params[:id])
  @membership.destroy
end

Then on the bottom:

private
def make_sure_user_has_ownership
  unless @logged_user.owns_membership?(@membership)
    # do whatever you want here (redirect, give the user a spanking)
  end
end

in the user model
def owns_membership?(membership)
  self.memberships.find_by_id(membership.id) ? true : false
end

To make it more DRY, you can also put a private action that calls the membership based on the id parameter for the show, destroy, edit, and update actions.

At the top

before_filter :get_club_membership, :only => [:show, :edit, :update, :destroy]

At the bottom (under private)
def get_club_membership
  begin
  @membership = ClubMembership.find(params[:id])
  rescue ActiveRecord::RecordNotFound
  # user's playing around with urls and the record doesn't exist
  end
end

If you do the last thing, you would have to remove the lines in the corresponding actions calling the membership row.