12

I'm programming an app (php) which requires a very long list of similar yet different functions, which are being called by a set of keys:

$functions = [
    "do this" => function() {
        // does this
    },
    "do that" => function() {
        // does that
    }
] 
etc.

I've chosen to place the similar functions in an array because they are not similar enough - getting the same result with one big function which is full of conditional statements isn't gonna work. And I do need to be able to call them only by key, for example:

$program = ["do this", "do that", "do this"];
foreach ($program as $k => $v) {
    $functions[$v]();
}

Thing is this functions-array structure is causing many problems, for example I'm having a hard time calling one array function from within another array function, e.g. this doesn't work:

"do that" => function() {
    $functions["do this"]();
}

nor this:

"do that" => function() {
    global $functions;
    $functions["do this"]();
}

or this:

"do that" => function($functions) {
    $functions["do this"]();
}

$functions["do that"]($functions);

I guess I could have one giant function with a long switch statement:

function similar_functions($key) {
    switch ($key) {
        case "do this":
            // does this
        break;
        case "do that":
            // does that
        break;
    }
}

But that doens't really seem like good practice. Or maybe it is?

So, what are my alternatives? Should I go with the switch structure? Or is there another, better solution?

Roy
  • 1,307
  • 1
  • 13
  • 29
  • `calling one array function from within another array function` won't work because the `$functions` will only be assigned its values after completely evaluating the RHS. – Nouphal.M Feb 11 '14 at 03:51
  • 1
    Store function name in a variable and then use variable as function. `$func = $functions[$v];$func();` – M Rostami Feb 22 '14 at 09:16

6 Answers6

20

Closures are expensive for performance and memoryusage in php. Procedural coding provoked a big ball of mud, spaghetti code and other anti patterns. Switch structures are very hard to test and it's a violation of OCP.

You should prefer OOP in SOLID way to avoid redundancy, improving scalability and maintainablity. That is the best practice to provide a set of functions which are reuseable. You can also seperate your code in layers and modules to reduce complexity and improve interchangability.

In your case your classes can implement __invoke to call it as invokable and your keys could be the fullqualified namespaces for these classes, so you can call it like a function. From now on you can also use inheritence, polymorphism or design patterns like composite, decorator etc. to reuse other functions or to add functions.

Here a simple Example..

<?php
use Foo\Bar;

class This
{
    public function __invoke()
    {
        $this->execute();
    }

    public function execute()
    {
        /* ... */
    }
 }

class That extends This
{
    public function execute()
    {
        /* ... */
    }
 }

$namespaces = array("\Foo\Bar\This", "\Foo\Bar\That"); 
foreach ($namespaces as $fullQualifiedNamespace) {
    /** @var callable $fullQualifiedNamespace */
    $fullQualifiedNamespace(); 
}

This behavior is also reachable by implementing a specific interface to This and That. Within iteration you can check the interface to call the defined contract. Or you build a Process Class where you can add objects which implements this interface. The Process class can execute all attached objects (Chain of Responsibility).

I would prefer the Chain of Responsibility Pattern this is understandable by most developers and not so magic like PHP's __invoke interceptor. In a factory you can define your chain and you are able to define other dependencies to the chain or to the attached chain objects.

Mamuz
  • 1,730
  • 12
  • 14
  • 4
    This is the best answer except for the syntax errors in the example :) 'do' can't be used for a method name! – satrun77 Feb 22 '14 at 05:40
  • Upps .. i renamed it to 'execute'. Thank you for the hint ;) – Mamuz Feb 22 '14 at 09:07
  • Really good answer. Off-topic, but I used a similar method to expose classes in a different application to a the application I built this for. It works well without wastefully using memory. – lukeocodes Feb 26 '14 at 09:07
  • By the way, why do you convince people to use SOLID, when there are other conventions for OOP, e.g. [GRASP](http://en.wikipedia.org/wiki/GRASP_%28object-oriented_design%29) should I mention at least one. What is SOLID better in than GRASP, or vice versa? – shadyyx Feb 27 '14 at 13:06
  • GRASP is a very good approach like Domain-Driven Design. But i think GRASP or DDD requires SOLID Principles. Goals of GRASP and DDD are reducing complexity and improving quick time-to-market, etc. In this case i refer to SOLID to focus to specific violations to provide a better alternative for this problem solving – Mamuz Feb 27 '14 at 15:41
7

You can do it in a cleaner way, that is, encapsulate all of these functions into a class. They can be static or dynamic, it doesn't matter much I guess in this case. I'll show both examples...

1. dynamic approach

class MyFunctions
{

    public function doThis()
    {
        /* implement do this here */
    }

    public function doThat()
    {
        /* implement do that here */
    }
    /* ... */
}

/* then somewhere else */
$program = ["doThis", "doThat", "doThis"];
$myFunctions = new MyFunctions;
foreach ($program as $method) {
    $myFunctions->$method();
}

2. static approach

class MyFunctions
{

    public static function doThis()
    {
        /* implement do this here */
    }

    public static function doThat()
    {
        /* implement do that here */
    }
    /* ... */
}

/* then somewhere else */
$program = ["doThis", "doThat", "doThis"];
foreach ($program as $method) {
    MyFunctions::$method();
}

IMHO this is cleaner than implementing those functions in an array of closures... And better than implementing a switch as You still have to call this in a loop - so why slowing down with another switch?

shadyyx
  • 15,825
  • 6
  • 60
  • 95
  • 1
    Dynamic approach is a violation of Single Responsibility Principle and growth to a godclass. Static approach is not a option for global state here. – Mamuz Feb 21 '14 at 23:13
  • I understand Your comments and I agree with You but on the other hand You should also think about the question and the problem - as You can see, the problem itself is caused by wrong architecture or wrong approach. Anyway I cannot find the requirement that the solution should work with Unit testing nor that it should follow SRP. – shadyyx Feb 22 '14 at 12:22
  • The question is "..what are my alternatives? ..is there another, better solution?". Your solution is better than Roy's approach, but has the same problems excepts reusing functions. What about adding new functions? What about functions which have a lot of dependencies? What about functions which needs arguments? If you have to resolve these problems you have to refactor everything. In the static approach nobody knows who can change the state. I think, if the problem is caused by wrong architecture you can even so solve the problem with good design. – Mamuz Feb 22 '14 at 17:04
  • Transfering the problems to one object is not really better but the first step to a better approach :) – Mamuz Feb 22 '14 at 17:11
  • OK, again, I agree, but the solution I provided is better then array of closures and just from the question itself (with his current setup using array of closures) it is clear there is no possibility to have and solve dependencies nor to call functions with different arguments. If his setup is running on array of closures then just slight modification and changing to a class is a big step better. And sometimes it is better to have 20-30 methods in one class (at one place) then having 20-30 classes more when no dependencies have to be solved... – shadyyx Feb 24 '14 at 08:45
3

I'm all in favor of what Mamuz describes in his answer, but I do want to give you a "quick fix":

$functions = [];

$functions['do this'] = function () {
    // does this
};

$functions['do call'] = function () use ($functions) {
    // calls 'do this'
    $functions['do this']();
}

$functions['do manipulation'] = function () use (&$functions) {
    // manipulates $functions
    $functions['do something else'] = function () {
        // does something else
    };
}

I think this is pretty self explanatory.

Community
  • 1
  • 1
Jasper N. Brouwer
  • 21,517
  • 4
  • 52
  • 76
2

What about the good old Command Pattern? ;)

<?php

interface Command{
    public function execute();
}

class DoThatCommand implements Command{
    public function execute()
    {
        echo "doing that...";
    }
}

class DoThisCommand implements Command{
    public function execute(){
        echo "doing this...";
        $doThatCommand = new DoThatCommand();
        $doThatCommand->execute();

    }
}

$program = [new DoThatCommand(), new DoThisCommand(), new DoThatCommand()];

foreach ($program as $command) {
    $command->execute();
    echo "\n";
}
?>

or in a more elegant dependecy injection way you can do it like:

<?php

interface Command{
    public function execute();
}

class DoThatCommand implements Command{
    public function execute()
    {
        echo "doing that... \n";
    }
}

class DoThisCommand implements Command{
    public function execute(){
        echo "doing this... \n";
    }
}

class DoTheThirdThingCommandThatDependsOnDoThisCommand implements Command{

    public $doThisCommand;

    public function __construct(DoThisCommand $doThisCommand)
    {
        $this->doThisCommand = $doThisCommand; 
    }

    public function execute()
    {
        echo "doing the ThirdThingCommand that depends on DoThisCommand... \n";
        $this->doThisCommand->execute();
    }
}

$command1 = new DoThatCommand(); 
$command2 = new DoThisCommand();
$command3 = new DoTheThirdThingCommandThatDependsOnDoThisCommand($command2);


$program = [$command1, $command2, $command3];
echo "<pre>";
foreach ($program as $command) {
    $command->execute();
}
?>
enterx
  • 869
  • 4
  • 14
2

Maybe something like:

/**
 * Save an array with function friendly names and its associated function and params
 */
$functions = array(
    'Do This' => array(
        'fn_name' => 'fn1',
        'fn_args' => array('fn1', 'from fn1')),
    'Do That' => array(
        'fn_name' => 'fn2'
    )
);

/**
 * Call this per each value of the functions array, passing along that function data as parameter
 */
function execute_fn($fn_data) {
    $fn_name = $fn_data['fn_name'];

    if(isset($fn_data['fn_args'])) {
        call_user_func_array($fn_name, $fn_data['fn_args']);
    } else {
        call_user_func($fn_name);
    }


}

function fn1($val1, $val2) {
    echo "I'm being called from $val1 and my second argument value is $val2 \n";
}

function fn2() {
    echo "Doing something for fn2 \n";
    fn1('fn2', 'from fn2');
}

/**
 * Call this to execute the whole set of functions in your functions array
 */
foreach ($functions as $key => $value) {
    execute_fn($value);
}

That way you can define in $functions an array with function names and argument values if that particular function expects any.

Gustavo Rubio
  • 10,209
  • 8
  • 39
  • 57
1

The giant function with the long switch statement is better because it allows you to call similar_functions("do this") as a part of similar_functions("do that"):

function similar_functions($key) {
    switch ($key) {
        case "do this":
            // does this
        break;
        case "do that":
            similar_functions("do this");
            // and then does that
        break;
    }
}
Sharanya Dutta
  • 3,981
  • 2
  • 17
  • 27