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 really like the ruby ? : syntax for if else statements.

However, I'm not sure how to do the following with a nested statement:

if information.is_empty?
  if leader? || possessor?
    link_to('update ' + contextual('your') + ' credentials!', edit_user_path) 
  else
    user.name + ' hasn\'t entered this info!'
  end
else
  information        
end

One reason I'm asking this question is should I bother? Does the ? : syntax (for want of a term!) actually make code pretty unreadable and non-self-documenting?

If you think I should use the ? : syntax, how would I tackle the above?

share|improve this question
add comment

2 Answers 2

up vote 10 down vote accepted
  1. The ?: is called a ternary operator. It's not specific to Ruby; a lot of languages have it (in fact, I believe I linked you to that wikipedia page in a comment very recently).

  2. Go easy on the newlines; there's a lot of whitespace there (update: question was edited; now it's gone)

  3. Use string interpolation

  4. We've talked about that is_ prefix on ? methods already :)

To answer your question: Ternaries have their use, but like everything else, they can also be abused.

In practice, there's no difference between a ternary and and a good ol' if...else, so you can use either - but that argument goes both ways. Don't use ternaries just because you can.

A good case for a ternary (in my opinion), would be something like this (just as an example):

puts some_boolean_value ? "yes" : "no"

or something like

"You have #{messages.count} new #{messages.one? ? 'message' : 'messages'}"

There's no logic going on in either the if or the else branch, and both are very short, so it's short and sweet to use a ternary.

In Ruby you can also spell the first one out like:

puts if some_boolean_value
  "yes"
else
  "no"
end

It's neat that Ruby treats if...else as an expression, but here it seems like a waste of space for something so simple.

In your case: Your code does have more branching logic, so I'd definitely avoid ternaries, and keep it as-is. Personally, I'd never nest ternary expressions inside each other, just for readability reasons.

Alternatively, you can return early (assuming this code is in a method):

return information unless information.is_empty?

if leader? || possessor?
  link_to("update #{contextual "your"} credentials", edit_user_path)
else
  "#{user.name} hasn't entered this info!"
end

If you wrote it all in ternaries, it'd be pretty ridiculous:

information.is_empty? ? leader? || professor? ? link_to("update #{contextual "your"} credentials", edit_user_path) : "#{user.name} hasn't entered this info!" : information # what?

The middle road would be to use a ternary for the innermost nested branch only

if information.is_empty?
  leader? || professor? ? link_to("update #{contextual "your"} credentials", edit_user_path) : "#{user.name} hasn't entered this info!"
else
  information
end

But, as you can see, that innermost nested branch is also the one with the most going on, so it's still less readable than a normal if...else

share|improve this answer
    
I think you did something strange in some of the examples; syntax highlighting looks wrong (although I don't know ruby) –  Izkata Apr 17 at 20:12
    
@Izkata Should be fine. The highlighting is just stumbling over the string-inside-a-string in the string interpolations. It's basically "text #{ruby code with a string in it} more text" and the syntax highlighter doesn't understand that –  Flambino Apr 17 at 20:26
add comment

I think that the key concern should be clarity. Specifically, I recommend eliminating the nesting, which would make it clear that one of three branches will be taken no matter what.

In addition…

  • Standard Ruby style uses two spaces of indentation.
  • is_empty? is a redundant name. The question mark alone is enough to convey the fact that it is a predicate.
  • Use less clumsy string quoting.
if !information.empty?
  information
elsif leader? || possessor?
  link_to("update #{contextual('your')} credentials!", edit_user_path) 
else
  "#{user.name} hasn't entered this info!"
end

You could write that using a ternary expression, but I would advise against it on readability grounds.

share|improve this answer
    
I see the leader? || professor? thing as orthogonal to whether there's any information. For a litmus test: You can't swap the branches around in your code and get the same result (not saying that's a great test, but just as an example) –  Flambino Apr 17 at 17:21
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.