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

Fix example error in memoization post #61

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@adarsh
Owner

adarsh commented Oct 13, 2016

Reason for Change

Changes

  • Remove that point for something gentler.
@gabebw

This comment has been minimized.

Show comment
Hide comment
@gabebw

gabebw Oct 13, 2016

Contributor

Friend-of-the-blog

That one's going in the twitter bio.

Contributor

gabebw commented Oct 13, 2016

Friend-of-the-blog

That one's going in the twitter bio.

@gabebw

This comment has been minimized.

Show comment
Hide comment
@gabebw

gabebw Oct 13, 2016

Contributor

👍 One additional point you may want to make, which ties in, is to use an underscore when memoizing in Rails controllers. Since all controller ivars are available in the view, It's often helpful to mark ivars that should only be used in the controller with a leading underscore. That also allows things like this, where @users is exposed to the view and @_users is just used for memoization:

def index
  @posts = posts.where(user: users.where(has_post: true))
  @users = users.where(confirmed: true)
end

private

def users
  @_users = User.all
end
Contributor

gabebw commented Oct 13, 2016

👍 One additional point you may want to make, which ties in, is to use an underscore when memoizing in Rails controllers. Since all controller ivars are available in the view, It's often helpful to mark ivars that should only be used in the controller with a leading underscore. That also allows things like this, where @users is exposed to the view and @_users is just used for memoization:

def index
  @posts = posts.where(user: users.where(has_post: true))
  @users = users.where(confirmed: true)
end

private

def users
  @_users = User.all
end
@adarsh

This comment has been minimized.

Show comment
Hide comment
@adarsh

adarsh Oct 13, 2016

Owner

@gabebw This is a marvelous point, and well-made.

I posted your comment verbatim into the article and cited you (b1a2e8f). Is this okay by you?

Owner

adarsh commented Oct 13, 2016

@gabebw This is a marvelous point, and well-made.

I posted your comment verbatim into the article and cited you (b1a2e8f). Is this okay by you?

> end
> ```
It's good to have smart friends.

This comment has been minimized.

@gabebw

gabebw Oct 13, 2016

Contributor

😊

@gabebw

gabebw Oct 13, 2016

Contributor

😊

@gabebw

This comment has been minimized.

Show comment
Hide comment
@gabebw

gabebw Oct 13, 2016

Contributor

Yes, of course! Looks great!

Contributor

gabebw commented Oct 13, 2016

Yes, of course! Looks great!

Fix example error in memoization post
Reason for Change
=================
* Friend-of-the-blog @gabebw astutely [pointed out how my irb example was not correct][1].
* In fact, the point I was making about adding to the public API is just incorrect.
* Add a great point about memoizing in controllers from @gabebw

[1]: #60 (review)

Changes
=======
* Remove that point for something gentler.

@adarsh adarsh closed this Oct 13, 2016

@adarsh adarsh deleted the ap-fix-ivar-error branch Oct 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment