views:

197

answers:

2

Unfortunately this mess works: Do you have suggestings for cleaning up this code: I'm trying to use a activerecord to compare two table columns, "needed" and "amount" then update a boolean column, depending on the returned data. Totally breaking do-not-repeat coding.

def update
    @inventory = Inventory.find(params[:id])

    respond_to do |format|
      if @inventory.update_attributes(params[:inventory])
      unless @inventory.needed.nil?
       if @inventory.needed < @inventory.amount then
       @inventory.instock = true
       @inventory.update_attributes(params[:inventory])
       else
       @inventory.instock = false
       @inventory.update_attributes(params[:inventory])
       end
      end
        flash[:notice] = 'Inventory was successfully updated.'
        format.html { redirect_to(@inventory) }
        format.xml  { head :ok }
      else
        format.html { render :action => "edit" }
        format.xml  { render :xml => @inventory.errors, :status => :unprocessable_entity }
      end
    end
  end

Cheers!

A: 

Very confused by your multiple calls to @inventory.update_attributes. You should only call that once when the records should be saved to the database. You should use @inventory.attributes = params[:inventory] and use if @inventory.save to hit the database.

Perhaps something like (untested):

@inventory.attributes = params[:inventory]
unless @inventory.needed.nil?
  @inventory.instock = (@inventory.needed < @inventory.amount) ? true : false
  @inventory.save
end
bensie
I'm not exactly sure where you actually want to call the save here since you had it in so many places :) You can add conditionals to re-render the form or redirect depending on its success as usual.
bensie
Basically if I run the logic before I update the DB, the logic won't function because the parameters aren't in the db. Therefor I updated the DB. Then the comparison was made, in which a second update was required, to enter instock into the db. Unless there's a way to test paramters, store it in a variable then update all 3 variables (instock, needed and amount) its the way that works. Its just REALLY messy and I know there must be a better way to do it. Thats why I've turned to stackoverflow! Thanks for your time on this, I think its an interesting problem and I hope to learn activerecord!
JZ
Can you explain the process here in words instead of code? There likely is a cleaner approach.
bensie
Sure - Basically, I'm a chemist trying to develop a custom inventory program for my academic department. And this is a simple inventory program we've been hacking at.In a form, a record is being updated, and the system will determine if an item is in stock based on the forms input. The basic input is the amount on hand (which changes every day) and the amounted needed (which is subject to change from time to time). Pretty simple really. Eventually I'd like to integrate this with my departments Purchase Order system, so determining if an item is in stock is a critical component to move fwd
JZ
+3  A: 

First, keep the extraneous logic out of your controller:

def update
  @inventory = Inventory.find(params[:id])

  respond_to do |format|
    if @inventory.update_attributes(params[:inventory])
      flash[:notice] = 'Inventory was successfully updated.'
      format.html { redirect_to(@inventory) }
      format.xml  { head :ok }
    else
      format.html { render :action => "edit" }
      format.xml  { render :xml => @inventory.errors, :status => :unprocessable_entity }
    end
  end
end

Then, handle the instock attribute with a callback on the Inventory model. Something like:

before_update :check_instock, :unless => Proc.new { |inventory| inventory.needed.nil? } 

def check_instock
  if needed < amount
    instock = true
  else
    instock = false
  end
end
PreciousBodilyFluids
Syntax looks right however I receive: ActiveRecord::ActiveRecordError "Callbacks must be a symbol denoting the method to call, a string to be evaluated, a block to be invoked, or an object responding to the callback method." Thanks for the advice in advance.
JZ
Sorry, I can't figure out what's wrong with it. My advice is to give it a couple hours and if nobody jumps in with the answer, ask a new question. I'd be curious to know the answer, too.If all else fails, you can simply use a symbol, though it's a little messy:before_update :check_instock, :unless => :needed_is_nil?privatedef needed_is_nil? needed.nil?end
PreciousBodilyFluids
looking at the check_instock, is that how you handle parameters (amount and needed) being passed within the Model?. Worst Case scenario my code above does work, its just messy. :/
JZ
I finally went back to this... this worked...def instock_logic if needed < amount then self.instock = 'f' else self.instock = 't' endend
JZ