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

Extend safe navigation docs about long &. chains #947

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

Conversation

fatkodima
Copy link
Contributor

Closes #946.

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

I love how you put it.

Considering the law of Demeter, long chains are inherently bad. But this principle has downsides, and they are due to forcing to write wrappers (like presenters), or propagate increasing the interface of the top-level receiver class.

It really depends. Can it be that an address doesn’t have a zip? It’s business logic, can vary.

This change seems for the best. Can you try finding confirmation that the suggested style is widespread?
Is there a cop already to facilitate finding this ?

README.adoc Outdated Show resolved Hide resolved
@fatkodima
Copy link
Contributor Author

Can you try finding confirmation that the suggested style is widespread?

At least in my company we are striving to adhere to this. I will try to investigate other codebases.

Is there a cop already to facilitate finding this ?

I am currently implementing this cop - rubocop/rubocop#13171

@fatkodima fatkodima force-pushed the safe-navigation-chain branch from 1c17c18 to 4f178e0 Compare September 11, 2024 18:15
@fatkodima
Copy link
Contributor Author

README.adoc Outdated
[source,ruby]
----
# bad
user&.address&.zip
Copy link
Member

Choose a reason for hiding this comment

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

I'm unsure if this example is appropriate. According to rubocop/rubocop#13228, up to two consecutive &. are allowed by default. It would be better to explicitly clarify, with an explanation and example code, how many consecutive &. are considered "Long safe navigation chain".

@pirj
Copy link
Member

pirj commented Sep 12, 2024

Rails code might not be representative, I believe they are strict in code quality.

How about other repos from real-world-rails-apps and real-world-ruby in general?

@bbatsov
Copy link
Collaborator

bbatsov commented Sep 12, 2024

We already have above the following rule:

Avoid chaining of &.. Replace with . and an explicit check. Perhaps it just needs to be expanded and clarified a bit instead of adding a new guideline?

@fatkodima fatkodima force-pushed the safe-navigation-chain branch from 4f178e0 to 3f97718 Compare September 12, 2024 15:45
@fatkodima
Copy link
Contributor Author

Perhaps it just needs to be expanded and clarified a bit instead of adding a new guideline?

Updated existing section for safe navigation. I am bad at writing docs - concrete suggestions welcome.

@fatkodima fatkodima changed the title Add section about long &. chains Extend safe navigation docs about long &. chains Sep 12, 2024
@bbatsov
Copy link
Collaborator

bbatsov commented Oct 20, 2024

Probably we shouldn't mention a specific number here and just expand a bit the rationale - namely that the longer the chain is, the harder it becomes to track what on it could be returning a nil. That's always been my fundamental problem with any safe navigation chains.

@fatkodima fatkodima force-pushed the safe-navigation-chain branch from 3f97718 to b845209 Compare October 20, 2024 18:01
@fatkodima
Copy link
Contributor Author

Updated with a suggested rationale.

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.

Limit the number of safe navigation calls in a chain
4 participants