Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This code smells to me, but I don't see the way to clean it up. I see v. mentioned over and over. Any hints would be appreciated!

  #
  # Create a new Value if it doesn't already exist, and initialize the attributes per
  # the parameters of the call.
  #
  def self.find_or_create_value(prog_id, part_id, round_id, quest_id, new_value=nil)
    prog = Program.find(prog_id)
    attrs = {participant_id: part_id, round_id: round_id, question_id: quest_id}
    v = prog.values.where(attrs).first
    if v.nil?
      v = prog.values.build
      v.assign_attributes(attrs, without_protection: true)
    end
    v.value = new_value
    v.save
    v
  end
end
share|improve this question
add comment

3 Answers

up vote 4 down vote accepted

You can use rails find_or_initialize_by method. Here is example:

  def self.find_or_create_value(prog_id, part_id, round_id, quest_id, new_value=nil)
    prog = Program.find(prog_id)
    prog.values.find_or_initialize_by_participant_id_and_round_id_and_question_id(part_id, round_id, quest_id) do |v|
      v.value = new_value
      v.save
    end
  end
share|improve this answer
add comment

Well I will repost my answer from stackoverflow for further discussion:

Ok here is a couple of things/smells that I notice about this:

  1. A method with over 4 parameters is a smell itself... that makes it unnecessarily complex. Maybe find objects that encapsulate some of the parameters (or: do you really need all of them?). Also with that many parameters you might want to put some of them in a hash (like your attrs hash) to remove the parameter ordering dependency.
  2. Use full names for variables etc.. prog --> program; v --> value etc. Increases readability by a lot. Code is read many more times than written.
  3. I would extract the code in the if statement into its own method so that you could put it into one line, kind of: value = some_method(prog, attrs) if value.nil? Or now that I think of it. You just try to create a value, in the if and in the line before. That could be a method of its own.
  4. Value seems to be tied to a specific program. Why not give program a method like create_value:

    def create_value(attrs, new_value = nil)
      # all the magic happens here
    end
    

Hope that helps :-)

share|improve this answer
    
Thanks... I just cross posted here per the suggestion over on SO. –  pitosalas Nov 10 '12 at 22:33
add comment

I'd write:

def self.find_or_create_value(prog_id, part_id, round_id, quest_id, new_value = nil)
  program = Program.find(prog_id)
  value_attrs = {participant_id: part_id, round_id: round_id, question_id: quest_id}
  value = program.values.where(value_attrs).first || program.values.new(value_attrs)
  value.value = new_value
  value.save
  value
end

Note that you lose the exit code of save but in your question is not clear how this should be handled.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.