# An abundance of ternary operators

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?

-
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

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

-
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.

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