55502f40dc8b7c769880b10874abc9d0

0 vote down star

Hello, so I have this big method in my application for newsletter distribution. Method is for updating rayons and i need to assigned user to rayon. I have relation n:n through table colporteur_in_rayons witch have attributes since_date and _until_date.

I am junior programmer and i know this code is pretty dummy :) I appreciated every suggestion.

def update
    rayon = Rayon.find(params[:id])
    if rayon.update_attributes(params[:rayon])
      if params[:user_id] != ""
        unless rayon.users.empty?
          unless rayon.users.last.id.eql?(params[:user_id])
            rayon.colporteur_in_rayons.last.update_attributes(:until_date => Time.now)
            Rayon.assign_user(rayon.id,params[:user_id])
            flash[:success] = "Rayon #{rayon.name} has been succesuly assigned to #{rayon.actual_user.name}."
            return redirect_to rayons_path
          end
        else
            Rayon.assign_user(rayon.id,params[:user_id])
            flash[:success] = "Rayon #{rayon.name} has been successfully assigned to #{rayon.actual_user.name}."
            return redirect_to rayons_path
        end
      end
      flash[:success] = "Rayon has been successfully updated."
      return redirect_to rayons_path
    else
      flash[:error] = "Rayon has not been updated."
      return redirect_to :back
    end
  end

Refactorings

No refactoring yet !

1f393f50dc29119d656c91d6701ddbba

Jeremy Weiskotten

May 8, 2010, May 08, 2010 21:49, permalink

No rating. Login to rate!

In the spirit of fat models and skinny controllers, I would move most that code into the Rayon model. You can use a virtual attribute to handle user assignment.

# MODEL
class Rayon < ActiveRecord::Base
  has_many :users

  # virtual attribute
  def user_assignment=(user_id)
    last_user_id = users.last.try(:id)
    if last_user_id.eql?(user_id)
      rayon.colporteur_in_rayons.last.update_attributes(:until_date => Time.now)
      assign_user(user_id)
    end
  end

  def assign_user(user_id)
    # TODO you can probably move the class method code to this instance method
    Rayon.assign_user(self.id, user_id)
  end
end


# CONTROLLER
def update
  @rayon = Rayon.find(params[:id])
  if @rayon.update_attributes(params[:rayon])
    flash[:success] = "Rayon has been successfully updated."
    redirect_to rayons_path
  else
    flash[:error] = "Rayon has not been updated."
    render :action => 'edit'
  end
end
5390dd26532b9591f3ea776bf07e46d9

Madison Amateur

April 29, 2011, April 29, 2011 00:12, permalink

No rating. Login to rate!

It is indeed a desirable thing to be well descended, but the glory belongs to our ancestors.

F80dd267381055d5f7077fb4a48925cc

CLOMIDONLINER

October 8, 2011, October 08, 2011 14:27, permalink

No rating. Login to rate!

<b>buy clomid 25mg in canada without prescription 1800</b> nolvadex or clomid
Sensitivity of Test : Different tests have different sensitivity thresholds. The more sensitive the test, the sooner you can test. Many drugstore brands require that you wait until your missed period to test. But this, in a way, sort of defeats the With a high sensitivity, early detection pregnancy test, you can learn if your are pregnant before your missed period. Tip : Use our FDA-Approved pregnancy tests which have the highest sensitivity levels and full two-year expiry dates. Lim SAT, Tsakok MFH. Age-related decline in fertility: a link to degenerative oocytes. Fertil Steril 1997;68:265-71. I can feel it. I knew that early with my first two pregnancies. This being my third pregnancy, I have a pretty good idea what to look for in my own body.

Your refactoring





Format Copy from initial code

or Cancel