Use a leading underscore for memoization

[BlakeWilliams]

When using an instance variable prefix the variable with an underscore
to discourage direct use of the ivar over the method.

[gylaz]

I haven't ever ran into a situation where someone used the memoization ivar. I could see this being a more relevant issue in controllers.

[BlakeWilliams]

I think we memoize controller methods for finding records pretty often. eg:

def user
  @_user ||= User.find(params[:id])
end

[gylaz]

I think we memoize controller methods for finding records pretty often

Exactly what I was referring to.

[jakecraige]

I like this. I've seen it before and it seems like a good idea.

[gfontenot]

fwiw, this is similar to the convention in Obj-C/Swift.

[teoljungberg]

I'm into it

[derekprior]

I've been doing this for a long time in Rails controllers because in controllers instance variables are public interface, not private state. In all other objects, instance variables are private state.

In the context of ruby objects, I've seen this confuse people and when I have to explain, "It's because rails controllers..." the counter is always, "but this isn't a controller". Outside of a rails controller, I don't see the need for this.

[derekprior]

That said, I do feel strongly that it's good to do in controllers and if people value consistency over my objections then I'm fine with that too.

[BlakeWilliams]

I ran into this outside of Rails when I had to perform an expensive operation to build paths that several parts of the gem would use, but may filter down further depending on the inputs. We ended up memoizing pre-transform for filtering, and post-transform when there was no filtering.

tl;dr I think it's applicable and good to use outside of Rails too.

[derekprior]

I don't understand the example you gave. Can you explain more? Was using the ivar directly a concern that adding a _ prefix fixed or are you talking about the benefits of memoization in general?

[BlakeWilliams]

Sorry, I started getting a little off-topic. The point I was trying to make is that this doesn't just apply to Rails controllers, it's just the place that we most commonly use it.

[jferris]

This doesn't just apply to Rails controllers

Within a class, is there risk of using the instance variable directly? If a method is defined for an instance variable, I think it's already considered best practice to use the method.

The main issue in Rails controllers is that you don't define methods for instance variables; they just show up in views. This makes it seem safe to use @user in a view, even though it's lazy-loaded, and you can't easily see if there's a method you should be calling instead, because you're in a different file than where the instance variable was assigned.

[mike-burns]

I always do this as a form of documentation: it's a sign to the reader that the i-var is used exactly once, for memoization.

[mcmire]

I do this too. I think it's a good idea because it indicates that the instance variable that's being used to store the memoized value is being used in a special way and isn't meant to be referred to (or isn't meant to be attr_accessor-ized and used that way). Because of that, I don't see this as a controller-specific guideline.

[gabebw]

I've been doing this for a long time in Rails controllers because in controllers instance variables are public interface, not private state. In all other objects, instance variables are private state.

I believe we should do this all the time in controllers, for exactly this reason.

[tommydangerous]

I like this a lot. I started adopting it at Airbnb.

[mcmire]

Okay so it sounds like we like this idea -- but do we like it just for controllers, or do we like it across the board?

[mike-burns]

I like it a "put something in here so that I can continue to use leading underscores for memoization in any method without having a fight about it every time" amount. I don't know how to express that in the guides, though.

[derekprior]

I retract my earlier objection to using this outside of controllers.

On Sun, Nov 29, 2015 at 5:32 PM, Mike Burns notifications@github.com
wrote:

I like it a "put something in here so that I can continue to use leading
underscores for memoization in any method without having a fight about it
every time" amount. I don't know how to express that in the guides, though.

—
Reply to this email directly or view it on GitHub
#373 (comment).

[jyurek]

I really dislike the look of this. IMHO, if we're going to stand on some ceremony for memoization, I'd much rather see a memoize method, e.g.

def user
    User.find(whatever)
end
memoize :user 

or even

memoize def user
    ....

Because then we don't have a specially coded ivar name in the code that people can just use anyway. If we're going to rely on conventions for "don't use this"-itude, we could just as easily not use ivars we use for memoization.

That said I'm all for doing what the group as a whole wants. I'm just rather surprised that no one's mentioned this possibility.

[mike-burns]

Where does #memoize come from?

[jyurek]

If we're going to standardize on something, we could write one and distribute it.

[mcmire]

Just want to point out that memoize was added to Rails a long time ago in the form of ActiveSupport::Memoizable, but then it was pulled. I suspect it had something to do with performance, but I'm not really sure why.

If we really want to use it, someone packaged it into a gem, memoist.

[backus]

(I watch this repo since you guys have a lot of great discussion, not sure how you guys feel about outside commentary so let me know if you'd prefer I just lurk in the future)

If you're considering gems for better memoization I'd recommend checking out https://github.com/dkubb/memoizable. Memoizable also has some of its own opinions which might be interesting for discussion regarding memoization. For example dkubb/memoizable#2 and dkubb/memoizable#6

[derekprior]

A gem seems unnecessary for cases that ||= handles just fine already, which is the overwhelming majority of uses of memoization, I think. Other techniques can be used when memoizing methods with parameters or memoizing boolean values, but those seem like memoization edge cases.

[backus]

Other big difference I think is that @foo ||= bar isn't a threadsafe operation

[BlakeWilliams]

Agreed, adding a gem for memoization feels like a little bit too much. The primary reason for this guide is to show the intention that the ivar isn't meant to be used and to discourage its use.

@backus I think you're right, that is unsafe in certain threading situations but there's also probably better approaches when you are in those situations than memoizing with an ivar on the first call of the method.

[mcmire]

A gem seems unnecessary for cases that ||= handles just fine already, which is the overwhelming majority of uses of memoization, I think

That's a good point, and that's probably the main reason why the Rails team decided to ultimately remove ActiveSupport::Memoizable.

I'm fine with just using ||=; it's easy enough to do.

[mike-burns]

I'd also be happy with this PR against best-practices/:

• Avoid memoization.

[mcmire]

Well, you should open a new PR for that then :) I'd be curious why you say that.

[JoelQ]

I'd also be happy with this PR against best-practices/:

Avoid memoization.

👍 I like that solution

[MattMSumner]

Is this still active? This seems like something many are doing but mostly in Rails controllers? If this was changed to Use a leading underscore when defining controller instance variables for memoization. I think this would be GTG.

This makes the most sense for rails controllers as instance variables are made available by default to templates. Adding the _ prefix gives a hint that the developer isn't expecting this instance var to be used outside the method defining it. Maybe this needs to be under style/rails rather then style/ruby?

[mcmire]

I'd rather this guideline be applied everywhere instead of controllers. For people who do not do this already and are new to this guideline, I think that would be easier to get into the habit of and remember instead of having to remember, oh this just applies to controllers. Doesn't that make sense?

[gylaz]

This confuses me, attr_accessor and friends use instance variables behind the scenes. If I choose to override a method, like:

def user_name
  @_user_name ||= randomize_user_name
end

I'm no longer using the same instance variable as attr_reader :user_name would have. This seems unexpected.

[mcmire]

@gylaz I think in that case you could reasonably choose not to prepend the variable name with an underscore. Is that difficult to remember?

[gylaz]

Is that difficult to remember?

I'm not sure what you mean by this. I could argue none of these guidelines are difficult to remember.

[mcmire]

What would you feel about keeping the following wording:

Use a leading underscore when defining instance variables for memoization.

and then remembering that if you are overriding attr_reader, then this guideline doesn't apply?

Or would you want to list that caveat?

[BlakeWilliams]

I think the intent is to name ivars that aren't meant to be used directly with a leading underscore. Would the following make more sense?

Use a leading underscore when defining instance variables that aren't meant to be used directly. e.g. memoization

[gylaz]

I don't fully understand what problem this solves for Ruby code outside of controllers. I don't often see code that uses the ivar directly when there is a getter method. Is the core argument here that when I see that leading _ it is supposed to communicate something to me (i.e. don't use this ivar directly)? Does out guideline of Prefer method invocation over instance variables. solve this already?

I'm not completely opposing it, I just want to understand the value of this guideline as well as generate some clarity and history around this decision.

[mcmire]

@gylaz You're right that this guideline would not serve a purpose outside of controllers. If you're not going to use the instance variables anyway, then it doesn't matter what they're called. However, if you're going to have a guideline and you want people to follow that guideline, then I think it's better to make it as simple to follow as possible. I am fine with having the guideline be "only use this in controllers", but I think the natural tendency will be to apply it everywhere, because if you are not following this guideline already then it will be easier to pick this up and start that habit. That's why I am voting to make it more generic.

If you don't think that will be an issue, then let's just make it specific and merge it.

[gylaz]

but I think the natural tendency will be to apply it everywhere, because if you are not following this guideline already then it will be easier to pick this up and start that habit.

That seems reasonable to me -- consistency. Though I dislike the fact that a Rails idiosyncrasy would dictate this rule across the rest of the Ruby code. I could see this suggestion be just as reasonable at Name the memoizing instance variable after the method name.

[mcmire]

Though I dislike the fact that a Rails idiosyncrasy would dictate this rule across the rest of the Ruby code

I actually agree, and if you combine "Prefer method invocation over instance variables" with "Render views with local variables instead of instance variables" (research card here) then this guideline isn't necessary at all, really.

I could see this suggestion be just as reasonable at Name the memoizing instance variable after the method name.

Yes -- if we opt to add this guideline, then this is a natural corollary, I think.

[mcmire]

I've just made a new PR for rendering views with locals, so we can talk about that there: #398

[teoljungberg]

I still like @derekprior's point of having memoization in a Rails controller will still "leak" state to the views. And denoting such cases with an underscore signals that "this is private data, don't touch it".

[BlakeWilliams]

Do we have a consensus on this? Good to merge?

[tommydangerous]

I like this. I started using it at Airbnb.

[MattMSumner]

How about Prefer a leading underscore?

[BlakeWilliams]

I personally lean towards Use, but if others think Preferis more appropriate I can update it.

[jferris]

From the README:

"Prefer" indicates a better option and its alternative to watch out for.
"Use" is a positive instruction.

Use makes more sense to me, as the alternative is just "don't use an underscore."

[jferris]

Is this still something we're interested in? Do people have objections to this pull request?

[tommydangerous]

I love it! No objections.

[MattMSumner]

@BlakeWilliams