Topic: How to clean this up

Hey Group,

This has been nagging me for a while and it seems if I try to keep light controllers & heavy models, there would be a better way to do this. I just don't see a huge benefit to this approach, I want to hear others thoughts, or if this is proper. I find myself doing all of it in the controller rather than creating an extra model method.

lets say I have a Movie model. I want to mark my movies as "available"

Currently I have a controller method called "mark_as_available"

def mark_as_available
 movie = Movie.find(params[:id])
 movie.available = true
 if movie.save!
   flash[:notice] = "Movie has been set as available."
 end
end

How could I clean this up and put it in the Movie model rather than the controller?  I'm assuming I could create a method called "mark_as_available" and simply call it like this...

Controller

def mark_as_available
movie = Movie.find(params[:id])
if movie.mark_as_available
   flash[:notice] = "Movie has been set as available."
end
end

Model

def mark_as_available
 self.available = true
end

Last edited by internetchris (2011-03-14 17:11:38)

Re: How to clean this up

that's correct but you never saved it, you just set the object's available column to true then left the object hanging until it goes out of scope. if you want to check to make sure your movie was marked as available and then save, maybe do

if movie.mark_as_available
  if movie.save
    flash[:notice] = "Movie has been set as available"
  end
end
- Ben

Re: How to clean this up

In case of save! condition is not necessary because it will raise an exception. If you want conditions in it please try 'save'.
Also you can use update_attributes:
def mark_as_available
movie = Movie.find(params[:id])
if movie.update_attributes(:available => true)
   flash[:notice] = "Movie has been set as available."
end
end
Also I think it will look better if you use inline condition:
flash[:notice] = "Movie has been set as available." if movie.update_attributes(:available => true)

Re: How to clean this up

I won't agree with KindBug
this is more descriptive:

flash [...] if movie.mark_as_available 
internetchris wrote:

Model

def mark_as_available
 self.available = true
end

should be

def mark_as_available
 self.available = true
 save
end

Last edited by rubytree (2012-08-08 18:06:21)