Private methods in subclasses incorrectly flagged as "never being called"

[jdickey]

I've seen PR #794 (and #724) and the issues they mention, and all the code I see seems to assume that the code being examined is either in a top-level class or module. Also, several releases have been tagged since #794 was merged into 3.8.0; this may be a regression.

I've written this up extensively in a Gist (with short code files attached) whose opening comment starts out as:

Tim and team,

? This should be pretty straightforward, but here goes. We use module namespaces and nested classes (too?) extensively in our project, but very rarely make direct use of inheritance. One place we do is illustrated by the following two files.

Prolog::Core::User::Validators::Name::Whitespace is a class that, hopefully obviously, defines a protocol to check for impermissible whitespace in a string. It has two methods, #invalid_spacing? and #reason, that must be overridden by subclasses that implement the protocol. Such a class is Prolog::Core::User::Validators::Name::TrailingWs; it implements only those two methods, which are declared private as documentation that they'll never be called ordinarily by outside code.

Reek doesn't like that:

(output follows in Gist)

Almost beside the point is that tests using the code show it works properly, and that Reek is so deeply confused it reports warnings against the class and each containing class. What gives here?

Thanks in advance for any attention this gets.

[troessner]

This is one of this detectors know limitations we can't fix. At least not without implementing a global class / method registry.

So for now I'm afraid you'll have to disable this detector either via source code comment or disable it completely in your config. Using exclude is not working right now unfortunately for this detector ( #816 )

We're thinking of disabling this detector by default in #844 - let's continue the discussion over there.
Closing this issue.

[mvz]

I would say this is exactly the scenario this detector warns against: Using private methods to provide plug-in functionality, so this particular issue is not due to Reek's (very real) limitation to single files.

The fact that it flags each container class/module as well is a bug. I'll open a separate ticket for that.

[troessner]

I would say this is exactly the scenario this detector warns against: Using private methods to provide plug-in functionality, so this particular issue is not due to Reek's (very real) limitation to single files.

But there are a lot of use cases where just defining a private method in the parent class and use it in its children has nothing to do with plug-in behaviour but rather is just good OOP style.

[mvz]

But there are a lot of use cases where just defining a private method in the parent class and use it in its children has nothing to do with plug-in behaviour but rather is just good OOP style.

The issue is with defining a private method in the parent class and only using it in its children. It sounds attractive at first, like a kind of 'toolbox', but in my experience it leads to an unmanageable mess very quickly.

By the way, the OP's case is the reverse: The method is defined in the child class to fill in a template method. I'm a bit less convinced this is a problem, although I would tend to use a collaborator object to provide the steps needed in the template.

[troessner]

The issue is with defining a private method in the parent class and only using it in its children.

Good point, I had missed this detail, thanks for the correction.