How to avoid short-circuit evaluation on

30
votes

I'm working with Ruby on Rails and would like to validate two different models :

if (model1.valid? && model2.valid?)
...
end

However, "&&" operator uses short-circuit evaluation (i.e. it evaluates "model2.valid?" only if "model1.valid?" is true), which prevents model2.valids to be executed if model1 is not valid.

Is there an equivalent of "&&" which would not use short-circuit evaluation? I need the two expressions to be evaluated.

6
ruby-on-railsoperatorslogical-operatorsshort-circuiting
Out of curiosity, why do you need both to be evaluated? Normally, if they are free of side effects, shortcircuiting is desirable. And why would #valid? have side effects? – Iraimbilanja
The two models are related to a same form : in the end, they'll have to be both validated to perform the action. There is no side effect, it's just because it's two models on a single form. – Flackou
Well... short-circuiting is exactly what you want. IF model1 is invalid, then why bother checking model2? – Iraimbilanja
To be fair, validating both models at once allows errors to be reported to the user on both models separately. It's annoying to only be notified about a validation error in model 2 only after you've fixed the model 1 error. – Paul Smith
For reference, #valid? does NOT notify the user of errors. It only populates the models errors array with errors. The controller then shows the new/edit view again which then shows the errors. – Samuel

6 Answers

26
votes

Try this:

([model1, model2].map(&:valid?)).all?

It'll return true if both are valid, and create the errors on both instances.

24
votes

& works just fine.

irb(main):007:0> def a
irb(main):008:1> puts "a"
irb(main):009:1> false
irb(main):010:1> end
=> nil

irb(main):011:0> def b
irb(main):012:1> puts "b"
irb(main):013:1> true
irb(main):014:1> end
=> nil

irb(main):015:0> a && b
a
=> false

irb(main):016:0> a & b
a
b
=> false

irb(main):017:0> a and b
a
=> false
4
votes

How about:

if [model1.valid?,model2.valid?].all?
  ...
end

Works for me.

2
votes

Evaluate them separately and store the result in a variable. Then use a simple && between those booleans :)

0
votes

One of the key concepts which allow making a code snippet more maintainable for a future developer is its expressiveness.

Let's consider the following examples:

([model1, model2].map(&:valid?)).all?

or

[model1.valid?,model2.valid?].all?

Both of them do their job well, but when a future developer will encounter any of them without seeing any explanatory comments, docs of your intent, or without reaching you directly, this developer will modify them having no idea that the purpose was to avoid the short-circuit evaluation.

Things become even worse when you have no tests.

This is why I suggest introducing a small wrapper method which will make all things clear immediately.

def without_short_circuit_evaluation(*conditions)
  conditions.all?
end

And later somewhere in your codebase.

if without_short_circuit_evaluation(model1.valid?, model2.valid?)
  # do something
end
-1
votes

Instead of creating an extra array with map, you can pass a block to all?.

[model_instance_1, model_instance_2].all? {|i| i.valid? }