Passing arguments to objects

[NotionCommotion]

Without posting a bunch of script, hopefully the following will describe my dilemma.

 

I have the following script.

$client = new \Client($db, new \Commands($logger), $logger);
$client->start();

Client's constructor will store Commands as $this->commands and will also poll the DB and save the results in another property $this->otherStuff.

 

Client also has a callback to a sockets connection, and given $cmd and $data, will perform if(method_exists($this->commands,$cmd)) { $this->commands->{$cmd}($data); }.

 

Now, my difficulty.  Some of the methods in Commands needs access to Client's $this->otherStuff.  As I see it, I have the following options.

 

1. Move Command's methods to Client.  Commands perform specific tasks, and this doesn't seem the right thing to do.
2. Create the Command's object within Client's constructor.  This seems to go against dependency injection.
3. Get otherStuff outside of Client, and pass it to both Client and Command.
4. Something else?

 

Hopefully this is enough information for some "general" recommendations.

 

Thank you

[Jacques1]

What does “Commands” do? What is “otherStuff”?

 

If you want a shot in the dark, I'll say you should pass your “otherStuff” to the Command method you call as additional context.

 

However, this is hardly a good approach to begin with. Right now, you don't even know which method you end up calling, which is fragile at best and dangerous at worst. $cmd could be some internal method that doesn't even have a parameter. There's no guarantee that it's one of your predefined “command methods”. To fix this, you should always call the same fixed(!) method which then maps $cmd to some action, using a closure or an actual method after checking a whitelist.

[NotionCommotion]

However, this is hardly a good approach to begin with. Right now, you don't even know which method you end up calling, which is fragile at best and dangerous at worst. $cmd could be some internal method that doesn't even have a parameter. There's no guarantee that it's one of your predefined “command methods”. To fix this, you should always call the same fixed(!) method which then maps $cmd to some action, using a closure or an actual method after checking a whitelist.

 

My rational for the Commands class was to ensure that only applicable methods exist in it, and it would have no internal methods.  If I really needed an internal method, I can make it not public and add to my method_exists() check whether the supplied command is public.  In hindsight, I guess that won't work as I have my constructor method to deal with.

 

Given the following script as a starting point, please elaborate on your statement "you should always call the same fixed(!) method which then maps $cmd to some action, using a closure or an actual method after checking a whitelist."   Thank you

<?php
$otherStuff=(object)['a'=>1,'b'=>2,'c'=>3];
$client = new \Client(new \Commands($otherStuff), $otherStuff);
$client->start();

class Client
{
    private $commands, $otherStuff;

    public function __construct($commands, $otherStuff)
    {
        $this->commands = $commands;
        $this->otherStuff = $otherStuff;
    }

    public function socketsCallBack($cmd,$data)
    {
        $map=['doA'=>'doAFunction','doB'=>'doBFunction','doC'=>'doCFunction'];
        if(isset($map[$cmd])) {
            $this->commands->{$map[$cmd]}($data);
        }
    }
}  

class Commands
{
    private $otherStuff;

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

    public function doAFunction($data){}
    public function doBFunction($data){}
    public function doCFunction($data){}
} 

[kicken]

If you really want to use method_exists rather than create a map of methods, create an interface that lists the valid methods then check for the method on that. That way your implementation can have other methods without them being possible targets for your dynamic call and you can enforce the signature of each possible call.

 

interface CommandsInterface {
    public function doAFunction($data);
    public function doBFunction($data);
    public function doCFunction($data);
}

class Commands implements CommandsInterface {
    public function execute($name, $data){
        if(method_exists(CommandsInterface::class, $name)) {
            $this->{$name}($data);
        } else {
            echo 'No such method';
        }
    }
}

[Jacques1]

I wonder why you need dynamic method calls in the first place.

 

If you have lots of short actions, use an array of closures:

<?php

class Command
{
    protected $commands = [];

    public function register($cmd, $action)
    {
        if (isset($this->commands[$cmd]))
        {
            throw new InvalidArgumentException('Command '.$cmd.' is already registered.');
        }

        $this->commands[$cmd] = $action;
    }

    public function execute($cmd, $data)
    {
        if (!isset($this->commands[$cmd]))
        {
            throw new InvalidArgumentException('Unknown command '.$cmd.'.');
        }

        $this->commands[$cmd]($data);
    }
}

// build the command instance
$command = new Command();
$command->register('a', function ($data) {echo 'Doing a.';});
$command->register('b', function ($data) {echo 'Doing b.';});
$command->register('c', function ($data) {echo 'Doing c.';});

// execute a command
$command->execute('a', ['foo' => 'bar']);

If you have lots of complex actions, it makes more sense to have decicated objects, similar to the Command Pattern.

 

If you have just a few fixed actions, use a plain old if statement which hard-coded method calls.