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

Multi condition case when #422

Closed

Conversation

NewAlexandria
Copy link
Contributor

Depends on Chanel #9

Following-up from the PR on When/Then block

There is no demonstration to confirm how to format when multiple conditions should result in the same action. I propose this:

# good

case token
when :star_op
  stack.pop * stack.pop
when :slash_op
  stack.pop / stack.pop
when :minus_op, :minus_minus_op
  also_calculate_that
  stack.pop - stack.pop
when MyModule::SomeDomain::BETA_USERS,
     MyModule::SomeDomain::INTERNAL_RELEASE
   stack.pop + stack.pop
when :int_literal,
     :some_complicate_explicit_name,
     :graduate_borrowers_with_arms,
     :str_interpolated
  token.value
end

Though better control of primary domain references should be exercised, this style offers a solution for some 'in the wild' situations. It reads better than:

# bad

case token
when :star_op
  stack.pop * stack.pop
when :slash_op
   stack.pop / stack.pop
when :minus_op, :minus_minus_op
  also_calculate_that
  stack.pop - stack.pop
when MyModule::SomeDomain::BETA_USERS, MyModule::SomeDomain::INTERNAL_RELEASE
  stack.pop + stack.pop
when :int_literal, :some_complicate_explicit_name, :graduate_borrowers_with_arms, :str_interpolated
  token.value
end

Where the 'bad' example also has the issue of cause the entire when line to diff when one of the conditions is changed or updated.

I know that some people love long lines, but I suggest that a newline is an easier-to-cognized demarcation of conditions, rathe than a comma

@freemanoid
Copy link

I believe you should update your PR considering the rule "Indent when as deep as case"
Correct version:

# good

case token
when :star_op
  stack.pop * stack.pop
when :slash_op
  stack.pop / stack.pop
when :minus_op
  also_calculate_that
  stack.pop - stack.pop
when :plus_op,
     :plus_plus_op
  stack.pop + stack.pop
when :int_literal 
  token.value
end

@bbatsov
Copy link
Collaborator

bbatsov commented Mar 28, 2015

The suggested rule is pretty subjective IMO - I don't see any notable readability improvements. I should also point out I've never seen it in the wild and I've been programming in Ruby for quite a while.

@freemanoid
Copy link

@bbatsov what is your suggestion for when statement with many conditions when it can't fit in one line?

@bbatsov
Copy link
Collaborator

bbatsov commented Mar 28, 2015

@bbatsov what is your suggestion for when statement with many conditions when it can't fit in one line?

Break them up as suggested here, of course. My problem with this PR is that it suggests doing this all the time.

@freemanoid
Copy link

As for me it's an obvious way to split conditions as suggested here but I believe that style guide must be explicit. We can add this rule with the note that it should only be used when conditions doesn't fit in one line. What do you think?

@bbatsov
Copy link
Collaborator

bbatsov commented Mar 28, 2015

What do you think?

Fine by me.

@NewAlexandria NewAlexandria force-pushed the multi-condition-case-when branch 4 times, most recently from 2aee498 to c68632c Compare March 30, 2015 00:32
@NewAlexandria
Copy link
Contributor Author

I've pushed an update to correct the indentation mistake.

I also tried to be more clear with in the wild situations that I've seen the need for this. I've actually been at more than one company that needed this formatting in the codebase.

I think it's lovely to imagine that every team is going to be agile and lean enough to get onboard with refactoring that can demonstrably improve maintainability. Sadly many orgs have complicated dependencies and Dilbert-grade managers. This part of the style guide helps in these situations.

I changed the examples to be permissive of short condition-groups that can stack and remain readable immediately — which I hope allays the concerns raised by those who do not often encounter the situation.

@freemanoid
Copy link

Why there're so many changes in PR? I believe you should include only multi condition changes in this PR. Could you explain it?

@NewAlexandria
Copy link
Contributor Author

woof, I don't know what all other others are; let me rebase. The only real commits here are

@freemanoid
Copy link

Could you cherry-pick these commits and squash them? I think it will help to merge this PR sooner.

@NewAlexandria NewAlexandria force-pushed the multi-condition-case-when branch from c68632c to 7c53f82 Compare April 1, 2015 14:33
@NewAlexandria
Copy link
Contributor Author

there we go 👍

@@ -326,6 +326,56 @@ Translations of the guide are available in the following languages:
end
```

<a name="multi-condition-case-when"></a>
* put multiple when conditions on separate lines

Choose a reason for hiding this comment

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

According to other rules in the guide:

  1. The sentence must be capitalized and end with a dot.
  2. Good and bad examples usually combined into single code snippet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@NewAlexandria NewAlexandria force-pushed the multi-condition-case-when branch from 7c53f82 to 2186b71 Compare April 2, 2015 19:29
@NewAlexandria
Copy link
Contributor Author

I've updated the good example to show both

  • all-on-one-line cases (when they are easy to read)
  • multi-line cases (when they are too-long, as discussed above)

@NewAlexandria
Copy link
Contributor Author

Is there more I can do to make this conformant with the ideals of this repo?

NewAlexandria added a commit to NewAlexandria/ruby-style-guide that referenced this pull request Mar 17, 2019
This replaces PR rubocop#422, and maintains a minimum of overall new lines
@NewAlexandria
Copy link
Contributor Author

Replaced by #741

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