Is assignment in a conditional clause good ruby style?

user_201893 0
Viewed 31597

In order to write more concisely, rather than do this:

test_value = method_call_that_might_return_nil()
if test_value
  do_something_with test_value
end

I've been assigning in the conditional:

if test_value = method_call_that_might_return_nil()
  do_something_with test_value
end

Is this bad style? The still-more-concise syntax:

do_something_with test_value if test_value = method_call_that_might_return_nil()

is not allowed, as discussed in another SO question, and will remain that way in 1.9, according to Matz (http://redmine.ruby-lang.org/issues/show/1141).

Given the possible confusion of assignment and comparison, does this make it too hard to read the code?

coding-style ruby
8 Answers

One somewhat widespread idiom is to use and, which would look something like this:

tmp = method_call_that_might_return_nil and do_something_with tmp

Another possibility would be to call #nil? explicitly, that way the intent becomes a little bit clearer; in particular it is really obvious that you actually meant to assign instead of compare:

unless (tmp = method_call_that_might_return_nil).nil?
  do_something_with tmp
end
user_29880

Concise code is not necessarily better code. Concision is useful when it improves the communication of intended code behavior from author to future maintainers. I think enough of us come from backgrounds in which we've had accidental assignments in if blocks (when we meant to have an equality comparison) that we prefer styles in which it's absolutely clear that assignment is meant, rather than comparison. The .nil? idiom already mentioned has that property, and I'd consider it cleaner than having the bare assignment inside the if condition. Really, though, I don't see the harm in having the extra line of code for the assignment.

user_2237830

The functional-programming way to do this is to use andand. It's a readable way of chaining method calls so that a nil in the middle stops the chain. So your example would be something like:

method_call_that_might_return_nil.andand.tap {|obj| do_something_with obj}
## or, in the common case: ##
method_call_that_might_return_nil.andand.do_something
Chuck_507420

Yeah, I would say it's bad style due to the possible confusion between assignment and comparison. It's only one more line to assign and then test, and it avoids having someone in the future think that was a bug and patch it to use == instead.

user_697550

C programmers do this a lot. I don't see a problem with it in Ruby either so long as it's clear what's happening.

user_667250

I think it's fine. Aversion to assignment in a condition comes from knowing that a missed key stroke when typing == turns a comparison into an unintended assignment. A stylistic prohibition on using assignment in a condition makes such accidents stand out like to the eye (and sometimes to the language, as in C, where many compilers can be made to emit a warning if they encounter an assignment in a condition). On the other hand, tests also make such accidents stand out. If your code is well covered by tests, you can consider discarding such prohibitions.

user_2388860

Due to the warning, performing the assignment in the if clause has a quite pungent smell. If you do have an else case to handle then the case ... in ... pattern matching can offer something:

case method_call_that_might_return_nil
in nil
  # handle nil case
in test_value # pattern match to a new variable 
  # handle non-nil case with return value of method assigned to test_value
end

Or...

case method_call_that_might_return_nil
in test_value if test_value # pattern match to a new variable if not nil
  # handle non-nil case with return value of method assigned to test_value
else
  # handle nil case
end
user_2788420
Related