Fix/refactor a few memoizations #4945
Conversation
|
|
||
| return current_role.resource if current_role&.resource&.is_a?(Organization) | ||
| return @current_organization = nil unless current_role | ||
| return @current_organization = current_role.resource if current_role&.resource&.is_a?(Organization) |
There was a problem hiding this comment.
Can we split up the assignment and return statements? Generally not a fan of one line doing two things.
There was a problem hiding this comment.
Yea that's fair, I was trying to keep the same code structure, but it isn't very clean like this.
| @@ -15,30 +15,31 @@ class ApplicationController < ActionController::Base | |||
| rescue_from ActiveRecord::RecordNotFound, with: :not_found! | |||
|
|
|||
| def current_organization | |||
| return @current_organization if @current_organization | |||
| return nil unless current_role | |||
| return @current_organization if defined? @current_organization | |||
There was a problem hiding this comment.
This makes me feel woozy... in general there being a difference between "defined but nil" and "not defined" seems very hand-wavy. I'd rather use a sentinel value (e.g. @current_organization = :no_org).
Another option is to use something like https://github.com/panorama-ed/memo_wise/ - or you can reuse the approach it uses, which is to have a top-level instance variable e.g. @memoized_results = {} and only set a result once it's attempted to be used, and make use of key? instead of comparing against nil.
There was a problem hiding this comment.
I don't know, I think using the defined? keyword (Ruby docs) is fairly common practice when memoizing falsey values. It's a pattern I've used before and I see it recommended in most places.
I took a look at a few implementations of memoization gems. Looks like memoist uses Object#instance_variable_defined?, a more specific version of defined? in order to memoize falsey values (source). memo_wise uses Hash#fetch to memoize falsey values (because that method differentiates between a key returning nil/false and a key not existing) (source).
Rails also uses this pattern (return @instance_variable if defined? @instance_variable) in a few places (source).
I think the hash approach of memo_wise makes sense when you are calling the method with arguments, so that you can hash results for different arguments passed in. But for our use cases (only memoizing methods with no parameters), it's just adding extra overhead.
I think adding another dependency to memoize ~5 methods is overkill, but it's up to you. If you say use a sentinel value or adding a gem I can do that. More important for me is that we actually memoize falsey values. (For example, in the partners portal, current_organization can evaluate to nil and can be called multiple times resulting in multiple db queries.)
Description
I noticed that the
current_organizationhelper method wasn't actually memoizing (it was never setting a@current_organizationinstance variable).Also noticed a few places where we were attempting to memoize, but not actually memoizing when the return value was falsey.
Type of change
How Has This Been Tested?
Automated test suite
Screenshots