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

Add rule against memoizing possibly falsely values #195

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

Conversation

sambostock
Copy link
Contributor

This adds a rule elaborating on some of the pitfalls of ||=.

"Avoid using ||= to initialize boolean variables" provides a good explanation about false not being safe to use with ||=, but cases where the value can be nil (e.g. not found) should also be considered.


Follow up to discussion in https://github.com/Shopify/ruby-style-guide/pull/193/files#r490432383

"Avoid using `||=` to initialize boolean variables" provides a good
explanation about `false` not being safe to use with `||=`, but cases
where the value can be `nil` (e.g. not found) should also be considered.
Copy link
Member

@rafaelfranca rafaelfranca left a comment

Choose a reason for hiding this comment

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

I don't think this can be a rule. What you are saying it is bad here can be totally fine if that is the logic you want in your code.

In reality, I hardly see cases where what the good example show is the one people should chose.

@volmer
Copy link
Contributor

volmer commented Sep 21, 2020

Agreed with @rafaelfranca. Since this was inspired by the existing rule about using ||= to assign boolean values to variables, we may need to rephrase the existing rule better in order to avoid confusion. In any case I don't think we can (or should) put restrictions about using ||= with nil. I don't see any stylistic problems erupting from that use.

Copy link

@PragTob PragTob left a comment

Choose a reason for hiding this comment

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

👋

I like this. I think I've never seen a case where you want the behavior that you want to execute it until you get a non nil response. And if that's the case then I'd argue that's why there's exceptions from the rule.

As a cache I most often see this as a lookup of some data. Let's say Shop.find_by name: name. The memorized method is then accessed all over, in loops and what not leading to n + m + x + y + z queries if done like this.

I also agree with Rafael that I haven't seen the good variant often (sell SimpleCov is full of it but for non memoization reasons...). I'd probably return some kind of result object (let's say for an API response the HTTP response with success/fail) or change the code to make the request one in the beginning and hand the variable through.

IMG_20180317_110254(1)

Base automatically changed from master to main March 15, 2021 17:09
@sambostock sambostock mentioned this pull request May 10, 2023
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.

4 participants