6
\$\begingroup\$

Given the following generic memoize utility for pure functions with type hints:

function memoize(callable $fn, callable $serialize = null): callable {
    $serialize = $serialize ? $serialize : 'serialize';

    return function(...$args) use ($fn, $serialize) {
        static $cache = [];
        $k = $serialize($args);
        return isset($cache[$k]) ? $cache[$k] : ($cache[$k] = $fn(...$args));
    };
}

Is the callable type hint here preferred as opposed to closure? I'm also not sure if I'm being too pedantic with the type hints and whether my use of static here is recommended?

I'd appreciate any feedback on the code above.

\$\endgroup\$
4
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mast Aug 9 '20 at 12:45
  • 1
    \$\begingroup\$ @Ngoy I've not seen this before, but had a quick look at how it is supposed to be used (eddmann.com/posts/implementing-and-using-memoization-in-php). Can you should me a [mvce] of how you are calling this function with sample data? \$\endgroup\$ – mickmackusa Aug 9 '20 at 20:36
  • 1
    \$\begingroup\$ @Mast sorry about that, I'll be sure to do a careful read of that post for questions going forward. Thanks for the feedback \$\endgroup\$ – Ngoy Lufo Aug 10 '20 at 18:41
  • 1
    \$\begingroup\$ @mickmackusa Here's a gist. It's a simplified version of a use case that I have. The main idea of the function is to allow one to pass a custom serialization function for the given arguments as some built in classes are not serializable. \$\endgroup\$ – Ngoy Lufo Aug 10 '20 at 21:18
3
\$\begingroup\$

To address your first question:

Is the callable type hint here preferred as opposed to closure?

There is a very similar question on StackOverflow: PHP: Type hinting - Difference between Closure and Callable. It has three answers.

The first line of the answer by Xorifelse explains:

The main difference between them is that a closure is a class and callable a type.

And the last line of the accepted answer by Rizier123 has this suggestion:

So if you only want to type hint anonymous function use: Closure and if you want also to allow normal functions use callable as type hint.


And now, for something completely different general code review comments:

Variable naming

The name $k is a little too brief... $key isn't very long and describes what the variable is.

Simplifying ternary expressions

Correct me if I am mistaken but as long as PHP 5.3+ is used then this line:

$serialize = $serialize ? $serialize : 'serialize';

could be shortened using the fact that:

The expression (expr1) ? (expr2) : (expr3) evaluates to expr2 if expr1 evaluates to TRUE, and expr3 if expr1 evaluates to FALSE.
Since PHP 5.3, it is possible to leave out the middle part of the ternary operator. Expression expr1 ?: expr3 returns expr1 if expr1 evaluates to TRUE, and expr3 otherwise.1

to the following:

$serialize = $serialize ?: 'serialize';

If PHP 7+ is used (which hopefully is the case, since at the time of writing, 7.2+ are actively maintained2) then the null coalescing operator i.e. ?? could also be used to simplify cases where a fallback should be used if a non-null value does not exist - e.g. :

 return isset($cache[$k]) ? $cache[$k] : ($cache[$k] = $fn(...$args));

Can become:

return $cache[$k] ?? ($cache[$k] = $fn(...$args));
\$\endgroup\$
4
  • \$\begingroup\$ Glad to be here, hope I'll get to enjoy it to. Thank you for answering my questions, I'll be sure to check out those answers on StackOverflow. You're right $key would be better and I didn't know about the shortened version of the ternary, that's gonna come in handy. \$\endgroup\$ – Ngoy Lufo Jul 31 '20 at 17:23
  • \$\begingroup\$ The Elvis Operator must not be used to check if a variable/element is "declared/set". For this you must use the null coalescing operator. Demonstration that the shorthand ternary is not appropriate and may produce Notices because it merely checks for truthiness: ideone.com/odUsIq \$\endgroup\$ – mickmackusa Aug 9 '20 at 22:26
  • \$\begingroup\$ @mickmackusa that is a good point and I updated my answer to mention it. The value should be set given the default value. If it was passed a value that wasn’t set then the notice would be thrown for the caller. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Aug 9 '20 at 23:11
  • 3
    \$\begingroup\$ I'd like to see a realistic usage of this memoizing technique so that I can understand what is going on. It looks like the functionality to is to allow the addition of data to the cache, but not mutate any existing data. I might yet post a review, but it seems like $fn would be better named $valueMutator and $serialize should be $keyMutator. The Elvis Operator is absolutely fine (though ?? is suitable as well) to use when declaring the fallback for $serialize; I was thinking about the $cache declaration when promoting the null coalescing operator. \$\endgroup\$ – mickmackusa Aug 9 '20 at 23:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.