Phpinspectionsea: Report anonymous functions on methods

[rentalhost]

It is a discussion/issue related to #199 and maybe with #203 (_I don't know_).

There are some sense of a method returns a lot of differents callables in a single method? I just see a unique case where it is allowed, where you should return an anonymous function as a complement for a function.

public function getCallable($type) {
    $function = function () { return 1; };

    if ($type === self::SOMETYPE) {
        return $function;
    }

    return function () { return 0; }
}

It should reports the getCallable() method by have two anonymous functions declared (independent of be returned or not). Briefly: if method have two or more anonymous function, report (you should reduces the responsability of method, reallocating this anonymous function to a named function in some place).

Then you could do that:

private function return0 () {
    return function () { return 0; };
}

private function return1 () {
    return function () { return 1; };
}

public function getCallable($type) {
    if ($type === self::SOMETYPE) {
        return self::return0();
    }

    return self::return1();
}

Note that the return0 and return1 methods just have a unique anonymous function (that does nothing, but not is that the point). While the getCallable() mehod returns this anonymous function "encapsulated" by another method.

[Fleshgrinder]

Still trying to wrap my head around that stuff. 😝

[rentalhost]

@Fleshgrinder what, exactly?

[Fleshgrinder]

The original Symfony example that was given in #199. Your example here seems over simplified, and the proposed solution seems over complicated.

public function getCallable($type) {
    return function () use ($type) {
        return $type === self::SOMETYPE ? 0 : 1;
    };
}

This looks like the proper solution to me. Testability is not an issue here, one can easily instantiate and call the method. On top of all that, the logic is so simple, not sure if I would even write a test for this.

[funivan]

@rentalhost how to deal with this code https://github.com/symfony/symfony/blob/7e766dae4a88be16dd5d7f4927deb032e0573a08/src/Symfony/Component/Console/Helper/ProgressIndicator.php#L240:L256 ?

[Fleshgrinder]

As I said, I need to wrap my head around that one still. Checked out Symfony yesterday, and will have a look at the implementation. The whole thing (just from looking at it on GitHub) looks very hacky and wrong.

[rentalhost]

@Fleshgrinder that was not the point, I just did a simplified example (and _yep, maybe I over simplificate too much a lot_ haha). About the testability, this is not the major issue here, is about SRP. There a lot of cases where you give responsability to method that does more than it should does. And I guess that an inspection like that should help dev avoid cases like that (example, _me_).

Your example is the best way to solve this problem, specifically. You just return an unique callable, what seems fine to me. The problem is when you returns differents callables in same method (as the Symfony case).

@funivan This code is a bit confuse, but I'll try (_hope that I am following the right logic_). I think about two options: if you do like to keep arguments private, you should declare some getter for that, else you should make it public.

_It'll show an example considering like public properties._

First I should create a specific class that will serves as a formatter. It should does exactly what the initPlaceholderFormatters() method does currently, but all code was moved to a specific class that I called ProgressIndicatorFormatter.

class ProgressIndicatorFormatter
{
    public static function elapsed(ProgressIndicator $indicator)
    {
        return Helper::formatTime(time() - $indicator->startTime);
    }

    public static function indicator(ProgressIndicator $indicator)
    {
        return $indicator->indicatorValues[$indicator->indicatorCurrent % count($indicator->indicatorValues)];
    }

    public static function memory(ProgressIndicator $indicator)
    {
        return Helper::formatMemory(memory_get_usage(true));
    }

    public static function message(ProgressIndicator $indicator)
    {
        return $indicator->message;
    }
}

The next step is rewrite the original method to run code from this class:

/** */
class ProgressIndicator
{
    /** */
    private static function initPlaceholderFormatters()
    {
        return [
            'indicator' => [ ProgressIndicatorFormatter::class, 'indicator' ],
            'message'   => [ ProgressIndicatorFormatter::class, 'message' ],
            'elapsed'   => [ ProgressIndicatorFormatter::class, 'elapsed' ],
            'memory'    => [ ProgressIndicatorFormatter::class, 'memory' ],
        ];
    }
}

At least for me, the code seems much cleaner and more directed to your responsibility. I still guess that it could be improved by avoiding cases like that (an array that returns a lot of callables). But for now, it seems better than original, and you have the advantage of specific a container to allow replace the default class.

For instance:

'indicator' => [ app(ProgressIndicatorFormatter::class), 'indicator' ],

[Fleshgrinder]

OK, had a closer look at the whole thing, and must say that it is very, very, very wrong from an OO perspective. We have global (static) state all over the place, as well as (very common in PHP land) primitive obsession. Solving the whole thing in a proper OO way requires lots of stuff, as is often the case in OO (and a reason why many people hate it). Nonetheless, here it comes.

https://gist.github.com/Fleshgrinder/2c523356101bed132d6548c3ed4d7735

The interfaces are mainly there to ensure that a user who just created an instance does not see any methods that are actually not callable at that moment. In other words, the IDE will show start only, and advance, finish, and display only after start was called; which is hidden until finish is called again.

I also exchanged the LogicExceptions with assertions and NOOP in case an incorrect call is made and the assertion is not executed. This ensures that programs in production do not break just because someone is misusing the API.

Everything is as immutable as possible. I also included the altered test to showcase that it actually works as intended.

Note that the ProgressIndicator class is still very easy to misuse. Something I thought about was to make RunningProgressIndicator an actual class, however, the question would be what to do after the finish call. The caller still has the object, and can continue calling methods on it. At the same time ProgressIndicator does not know that finish was called, or anything actually, hence, it is hard to protect against someone calling start multiple times. The only way to achieve this would be through a lock on the output. I did not want to go that far.

PS: I prefer to use named constructors (the static new methods) instead of the actual constructor because it forced the arguments to be invariant in case of extension through other classes, and avoid multiple constructor calls. Probably something I picked up from Rust.

[kalessil]

So, 2 years later: I see the point. Alternative of polluting global scope is still a big disadvantage as increases overall complexity (DI, contracts and etc. for the formatter example). When it comes to testability, both techniques are testable.

And the last factor, which forces me to decline this ticket: JS/.NET/Java communities are heavily adopting such techniques, so PHP will do it a well =)