Add rule against memoizing possibly falsely values#195
Add rule against memoizing possibly falsely values#195sambostock wants to merge 1 commit intomainfrom
Conversation
"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.
rafaelfranca
left a comment
There was a problem hiding this comment.
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.
|
Agreed with @rafaelfranca. Since this was inspired by the existing rule about using |
PragTob
left a comment
There was a problem hiding this comment.
👋
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.

This adds a rule elaborating on some of the pitfalls of
||=."Avoid using
||=to initialize boolean variables" provides a good explanation aboutfalsenot being safe to use with||=, but cases where the value can benil(e.g. not found) should also be considered.Follow up to discussion in https://github.com/Shopify/ruby-style-guide/pull/193/files#r490432383