Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow use of and/or operators for control flow #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

davepilling
Copy link

Copy link
Member

@boardfish boardfish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does our RuboCop config support this too?

@davepilling
Copy link
Author

Does our RuboCop config support this too?

We'll need to update this rule:

Style/AndOr:
  EnforcedStyle: always

to:

Style/AndOr:
  EnforcedStyle: conditionals

```ruby
# good: and/or for control flow
x = extract_arguments or raise ArgumentError, "Not enough arguments!"
user.suspended? and return :denied
Copy link

@fran-fac fran-fac Jan 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we want to use...

user.suspended? and return :denied

instead of a guard if?

return :denied if user.suspended?

How do we choose? A bit wary of opening up a "two ways to do one thing" situation, when the existing codebase is consistently using a form that looks just as powerful.

The feeling I have is that and/or are more readable when the method call reads like an action:

do_something or return :failure

while guard if/unless work better with boolean methods:

return :invalid unless is_valid?(param)

However, the first case requires the method to return a truth-y value, which feels like... I suppose it reminds me of PHP, where most methods return false|value and and/or are often used for control flow – but there are not guard ifs in PHP!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would go with the return...if if that case. I do think think this is a poor example when you could use a guard if instead.

As you say, I think and/or work better when you're checking whether an action was successful. You can achieve the same thing with if/unless but I find that is actually less readable in the action case:

do_something or return :failure

return :failure unless do_something

Happy to modify the examples if you think that pattern makes sense.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In our examples above, do_something needs to return a truth-y value, but does not guarantee a boolean as do_something? would.

Are we putting a invisible dependency on the called method then, and risk someone changing do_something in a perfectly valid way, which breaks the implicit truth-y-ness return requirement? Is that an actual risk, and is it acceptable in respect to what we gain from relaxing the rules around and/or?

(Maybe the above is not very clear, I'll try again with a fresh brain tomorrow)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants