False InstanceVariableAssumption on memoization

[texpert]

The ivar memoization with 'if defined?' is giving a false positive 'InstanceVariableAssumption: GoogleAuth assumes too much for instance variable '@refresh_token'.

attr_reader :refresh_token

def refresh_token
  return @refresh_token if defined? @refresh_token
  @refresh_token = auth.credentials['refresh_token']
end

[mvz]

Yes, Reek unfortunately cannot detect this style of memoization. This is mainly since there are just so many different ways to phrase it.

That said, if you don't expect auth.credentials['refresh_token'] to return nil you can use @refresh_token ||= auth.credentials['refresh_token'] instead (If only there were such a concise way for checking with defined?!).

(Is there a particular reason you're including attr_reader :refresh_token? I don't think it makes a difference.)

[texpert]

I certainly know about ||= but this is the case with possible nil.

Regarding the attr_reader :refresh_token it was long since refactored as long as the whole bunch of code. But there are such memoizations in other places too.

[mvz]

Ok, thanks for clarifying. We'll need to put some thought into how we can detect this case.

[jakswa]

:+1: for this one. I spent a good while trying to refactor away from this memoization style, because of reek. Recently I became fed up enough to dig into a more-correct pattern, only to find out that my code shouldn't reek. :smile:

edit: This is the first case I've needed to do this, but if anyone else is as new as me, you can put this on your class definition, to disable the reek:

# Suppressing incorrect smell of :reek:InstanceVariableAssumption
class ...

[mvz]

Another workaround may be to use one of the existing memoization gems.

[Drenmi]

Recently I became fed up enough to dig into a more-correct pattern, only to find out that my code shouldn't reek.

Do you mind sharing your research? At first glance, "defined checks" seems like a lower circle of hell than "nil checks" to me, but it sounds like you found something that made you decide that this is fine. I haven't really encountered code like that myself, so asking mostly out of curiosity. 🙂

[jakswa]

Do you mind sharing your research?

When I became disheartened by my flailing refactor attempts, I only found this thread, which seems to state that it's a bug upwind, in the nose. As far as I know, there isn't a way around using defined?, if you want to ensure a possibly-falsey-returning statement is only called lazily, and once.

"defined checks" seems like a lower circle of hell than "nil checks" to me

I feel like I lost you here. Neither of these two memoization patterns deal with a nil? check, that I can see. I've always thought of ||= as a falsey check. I'd welcome a nil? check if it works around any of the things I'm smelling, but I don't see how it would. Do you think a defined check (or memoization) is the wrong solution? I'm open to ideas, for sure.