3b583d1c5e2d57db5ad32ee82b56adf4

This is a custom method in my controller to see if a job already has a specific material... if it does, update the quantity, otherwise add the material.

It just feel bad. Would love to know how to do this in more of a rails way.

def update_materials
    jm = JobMaterial.find(:first, :conditions => {:material_id => params[:job_material][:material_id], :job_id => params[:job_id], :location => params[:job_material][:location]})
    if jm
      if jm.update_attribute(:material_quantity, jm.material_quantity + params[:job_material][:material_quantity].to_i)
        flash[:notice] = "The material count has been updated"
      else
        flash[:error] = "Something got screwed up and your material count was not updated"
      end
    else
      jm = JobMaterial.new({:material_id => params[:job_material][:material_id], :job_id => params[:job_id], :location => params[:job_material][:location], :material_quantity => params[:job_material][:material_quantity]})
      if jm.save
        flash[:notice] = "The materials have been added"
      else
        flash[:error] = "Something got screwed up and your materials were not added"
      end
    end
    redirect_to job_job_materials_path(params[:job_id])
  end

Refactorings

No refactoring yet !

3b583d1c5e2d57db5ad32ee82b56adf4

kagedwolf.myopenid.com

February 16, 2010, February 16, 2010 22:32, permalink

No rating. Login to rate!

I figured out how to use find_or_create_by_

One thing that is a little wonky is dealing with a material_quantity = nil in a newly created JobMaterial record.

Also, there must be a nicer way of dealing with the params[:job_material][:material_id] stuff. Something similar to JobMaterial.new(params[:job_material]).

def update_materials
    jm = JobMaterial.find_or_create_by_material_id_and_job_id_and_location(params[:job_material][:material_id], params[:job_id], params[:job_material][:location])
    jm.material_quantity = jm.material_quantity.to_i + params[:job_material][:material_quantity].to_i
    if jm.save
      flash[:notice] = "The materials have been added"
    else
      flash[:error] = "Something got screwed up and your materials were not added"
    end
    redirect_to job_job_materials_path(params[:job_id])
  end
D41d8cd98f00b204e9800998ecf8427e

steved

February 16, 2010, February 16, 2010 23:40, permalink

1 rating. Login to rate!

Not so bad. I'd push more of the logic into the model and make use of #increment! with default value of 0

# in migration define column as not null, default 0 so new rows have 0 instead of null

create_table :customer_networks, :force => true do |t|
  t.integer :material_quantity, :null => false, :default => 0
  # rest of columns
end

# in controller

def update_materials
  if JobMaterial.find_or_create_and_update_quantity(params)
    flash[:notice] = "The materials have been added"
  else
    flash[:error] = "Something got screwed up and your materials were not added"
  end
  redirect_to job_job_materials_path(params[:job_id])
end

# in JobMaterial model

def self.find_or_create_and_update_quantity(params)
  material_id = params[:job_material][:material_id]
  job_id      = params[:job_id]
  location    = params[:job_material][:location]
  job_material = JobMaterial.find_or_create_by_material_id_and_job_id_and_location(material_id, job_id, location)

  additional_quantity = params[:job_material][:material_quantity].to_i
  job_material.increment!(:material_quantity, additional_quantity)
end

Your refactoring





Format Copy from initial code

or Cancel