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.

I'd like to find a way to do what this method does in a cleaner, less hacky looking way

 def self.create_from_person!(person)
    spi = new(:person => person, :provider => person.provider)
    spi.age_eligible_code = participant.age_eligible?(person) ? 1 : 2
    spi.county_of_residence_code = participant.psu_county_eligible?(person) ? 1 : 2
    spi.first_prenatal_visit_code = participant.first_visit?(person) ? 1 : 2
    spi.pregnancy_eligible_code = participant.pbs_pregnant?(person) ? 1 : 2
    spi.save!
  end

Any thoughts?

share|improve this question
    
where does participant come from? –  tokland Feb 8 '13 at 16:21
    
@tokland participant is from a different model. I have some doubt that the eligibility methods belong there instead of on Person but thats not directly related to the feature I am working on. In case your interested the Participant model and the rest of the project is here I like the refactoring below and I'm thinking about submitting it as part of my commit, if thats okay with you –  steve_gallagher Feb 8 '13 at 16:26
    
It's ok, of course. But I still don't see where is participant defined. To be called from a classmethod it should be a classmethod as well, but with that name it doesn't look like one. –  tokland Feb 8 '13 at 16:42
    
ah, right. its a bug with my implementation from above. Its defined as an association on person so there should be a line above that is like participant = person.participant. Thanks. –  steve_gallagher Feb 8 '13 at 17:23
    
ok! that's what it seemed, a local variable yet to be defined. –  tokland Feb 8 '13 at 17:34

2 Answers 2

up vote 5 down vote accepted

Notes:

  • Any reason not to set the attributes in a single create step?
  • DRY by using procs for short-lived functions.
  • Methods should return meaningful values. Here it makes sense to return the newly created object.
  • (As Mark pointed out) The mismatch names between attributes makes the code more verbose that it should be. However, I guess part of this mismatch already comes from the app you are patching.
  • :provider => person.provider. Note that this can break referential integrity. Person may change its provider in the future, but this model will remain as it is.

I'd write:

def self.create_from_person!(person)
  code = lambda { |value| value ? 1 : 2 }
  create!({
    :person => person, 
    :provider => person.provider,
    :age_eligible_code => code[participant.age_eligible?(person)],
    :county_of_residence_code => code[participant.psu_county_eligible?(person)],
    :first_prenatal_visit_code => code[participant.first_visit?(person)],
    :pregnancy_eligible_code => code[participant.pbs_pregnant?(person)],
  })
end
share|improve this answer
    
any reason(s) for the -1 so I can improve the answer? –  tokland Feb 10 '13 at 19:35

The reason why it doesn't look clean is because the underlying models are inconsistent.

In one, you return a boolean true/false for age_eligible? and in another, you call it age_eligible_code and store a 1 for true and 2 for false. If these were aligned such that they were both boolean values, you wouldn't need any ternary operators at all.

share|improve this answer
    
Indeed, also the ":provider => person.provider" may create inconsistencies in the SQL tables. –  tokland Feb 12 '13 at 13:47

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.