Placement of success code in a conditional

Question

I'm not sure if there's a standard view where the placement of a success result should be in a conditional that can return multiple statuses. The success condition of this function is in the middle of the conditional block:

def redeem(pin)
 success = false
 message = nil
 if !self.date_claimed.nil?
 message = 'Reward already claimed'
 elsif self.reward.redemption_pin == pin
 success = true
 self.date_claimed = Time.zone.now
 self.save
 else
 message = "Wrong pin"
 end
 return {:success => success, :message => message}
end

I could rewrite it to be at the end:

def redeem(pin)
 success = false
 message = nil
 if !self.date_claimed.nil?
 message = 'Reward already claimed'
 elsif self.reward.redemption_pin != pin
 message = "Wrong pin"
 else
 success = true
 self.date_claimed = Time.zone.now
 self.save
 end
 return {:success => success, :message => message}
end

or I could rewrite it to be at the top but conditions become slightly more complex:

def redeem(pin)
 success = false
 message = nil
 if self.reward.redemption_pin == pin && self.date_claimed.nil?
 success = true
 self.date_claimed = Time.zone.now
 self.save
 elsif date_claimed
 message = 'Reward already claimed'
 else
 message = "Wrong pin"
 end
 return {:success => success, :message => message}
end

I personally think I prefer it to be at the beginning or the end, but in this function the end seems to be the best place.

Answer 1

Some notes:

• Imperative programming has its use cases (or so I've heard :-)) but implementing logic is definitely not one of them. Some thoughts on the matter. Use always expressions, not statements, to describe logic; that's it, don't begin with a x = 1 and then modify x somewhere else in the code. Write expression branches instead (conditionals are in expressions in Ruby). Don't think in terms of "how" but in terms of "what".

• if !x.nil? -> if x.

• obj.save may fail but you are not checking it.

• Don't write explicit return (more on idiomatic Ruby).

• As you say the order of checks is important. I usually check for "problems" first, and keep the "ok" scenario for the last branch.

• You are using a value (a hash) instead of an exception to signal errors. It's slightly not idiomatical in Ruby (in the sense that people tend not to do it, not that there's anything wrong with it), but personally I like it (and use it).

You could write:

def redeem(pin)
 success, message = if date_claimed
 [false, 'Reward already claimed']
 ...
 end
 {:success => success, :message => message}
end

However, returning {success: ..., message: ...} on each branch, yet slightly more verbose, looks pretty nice and declarative:

def redeem(pin)
 if date_claimed
 {success: false, message: "Reward already claimed"}
 elsif reward.redemption_pin != pin
 {success: false, message: "Wrong pin"}
 else 
 update_attribute(:date_claimed, Time.zone.now)
 {success: true}
 end
end

• Omar: I wasn't aware that I could assign variables via an array like that. Thanks a ton for the thorough answer and explanation. I also like the second style more.

Answer 2

I don't like this

success = false

at the beginning.

You can remove it, and later in your code use the double bang to get a boolean value

return {:success => !!success, :message => message}

I mean get rid of lines that didn't provide any functionality, in this case you will save a line, remaining the reability of the code.

And when you set success to true, always write the condition for it, when it should be true. In your second example, you mark your method as sucessfull in an else block. That means for me: "If none of the previous conditions met, I'm sucesful", but what happens, when aditional failure scenario will be written above the else block, or other stuff happen above the else block ?

You definately should precise describe the conditions for a sucess state (like example 3)