Skip to content

Limit use of conditional modifiers to short, simple cases. #738

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+92 −1
Open

Limit use of conditional modifiers to short, simple cases. #738

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion ruby/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,13 @@

[Sample 1](sample_1.rb) [Sample 2](sample_2.rb)

> **TIP**
>
> Click on the linked pull request, commit, or the guideline itself to read more
> detailed explanations with examples and reasoning behind these recommendations.

- Use [standard]
- Avoid conditional modifiers (lines that end with conditionals). [36491dbb9]
- [Limit use of conditional modifiers to short, simple cases.](./conditional_modifiers.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: I think the file name should match the link label

  • when they match, it lowers cognitive cost of coming up with both the title and filename
  • it's easier to locate the correct file when the filename matches the title
  • using the title as the filename is more unambiguous and natural to use.
  • would facilitate consistency in naming files
  • The file name is more likely to be unique, making it easier to generate links in markdown editors like Obsidian automatically suggest pages to link to
  • Differentiates between other files that may also speak about conditional modifiers
  • This is similar to what happens on Wikis where the title and file name are the same
  • Using a propositional title (at least for those notes where the title forms a sentence or independent clause; such as statements, assertions, declaratives, advice, claims, theories, and/or imperatives) is useful for forming interconnections between pages because the filename/title forms a concept handle. Concept handles can then serve as an abstraction of the page/note itself. Which then facilitates composing and mixing concepts together to generate new ideas.
  • I would expect the title "Conditional Modifier" (a lexical title) (singular not plural on purpose) to be a page about the concept or object itself. I'd expect this noun phrase to be a definition or encyclopedic knowledge on what a conditional modifier is.

- Avoid multiple assignments per line (`one, two = 1, 2`). [#109]
- Avoid organizational comments (`# Validations`). [#63]
- Avoid ternary operators (`boolean ? true : false`). Use multi-line `if`
Expand Down
86 changes: 86 additions & 0 deletions ruby/conditional_modifiers.md
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be interested in finding a standardized layout, form, or pattern (informal definition) for guideline pages. Something akin to Pattern Forms. Though I acknowledge that we aren't generating formal patterns here, but more sharing best practices, maxims, preferences, conventions, heuristics, lessons learned, etc.

Still, I would argue that creating a structural consistency across guideline pages would facilitate the readability, scanability, searchability, navigability, and organization of the pages. This would encourage internal consistency across a diverse set of guidelines.

I'm not sure what this form would look like (we might need to build out several pages first and discover the common pattern). Perhaps something like: problem, solution, tradeoffs (indications/contraindications; pros/cons; advantages/disadvantages).

I could also see us using a variant the portland form as well...

  • opening statement — roughly cover the description, problem, context, and forces to be resolved
  • therefore... — thesis, solution, the action to take, resolution, resulting context
  • but... — antithesis, contraindications, exceptions, things that contrast the concepts already mentioned

It enacts just enough structure to assist with pages being consistent while still being ad hoc and flexible.

Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
# Limit use of conditional modifiers to short, simple cases

Conditional modifiers (i.e., `if` or `unless` at the end of a line) can be
surprising when they appear on long or complex lines. The reader might not see
them while scanning the code.

So, prefer to use them only for short, simple cases. For example:

```ruby
do_later if async?
```

The example above can read more naturally than:

```rb
if async?
do_later
end
```

## Complex conditions

However, if the line is too long (around 80 characters) or complex (e.g., an
`if` with multiple conditions like `if a && b`) prefer the multi-line form:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is especially true with unless since you an introduce a DeMorgan bug


```ruby
# Avoid
block_access! if signed_in? && !current_user.active?

# Prefer
if signed_in? && !current_user.active?
block_access!
end
```

There might be cases where the conditional modifier work well with multiple
conditions, so use your best judgment.

## An opportunity to refactor

If the conditions are related, consider extracting a method that groups them.
This might allow you to use the conditional modifier form again.

```ruby
def inactive_user?
signed_in? && !current_user.active?
end

block_access! if inactive_user?
```
Comment on lines +39 to +50
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be moving towards the Triangle of Separation style of code


## Conditional modifiers feel informal

The modifier form of conditionals can feel more casual than the multi-line form.
Conversely, the multi-line form _draws attention_ to the conditional and the
code that follows it. Use this to your advantage when you want to emphasize the
conditional and the code that follows it.

```rb
# Avoid
def action
return destroy_all if really?

do_nothing
end

# Prefer
def action
if really?
destroy_all
else
do_nothing
end
end
```

You can also refactor the code so the less destructive action uses a conditional
modifier, which pairs well with the informal feel of the modifier form:

```rb
def action
return do_nothing if chill?

destroy_all
end
```
Comment on lines +52 to +86
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this section is really about what is the "main action" of the method, and what is the edge-case blocked by the guard close. It seems less about formality and more about verifying invariants?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd say both conditions can be equally valid, which is highlighted in the if/else example? The writer has the power to bring focus to particular branch by not using the guard. Maybe that's what I was trying to communicate with formal/informal.