0
\$\begingroup\$

I have written Command Class & CommandCollection class.

Every command will have instance of Command object and it get assigned in the CommandCollection object.

Usage:

$commandCollection = new CommandCollection();

$data = ['domain' => 'www.domain.com'];

// version 4 and 5 is belong to script_one.sh file under add-site name
$commandCollection->addCommand('add-site', 'path/script_one.sh', $data)
                   ->forVersion([4, 5]);

$commandCollection->addCommand('add-site', 'path/script_two.sh', $data)
                   ->forVersion([6]);

View the shell script of version 5 (path/script_one.sh)

echo $commandCollection->viewCommand('add-site', 5);

I will be using CommandCollection object in some of Manager classes such as SitesManager class and CronJobManager class

What do you think of my code and is there anything could be improved or refactored?

CommandCollection Class

class CommandCollection
{
    private $commandFiles;

    public function __construct()
    {
        $this->commandFiles = collect([]);
    }

    public function addCommand($name, $filename, $data)
    {
        $nameFilter = $this->commandFiles->where('name', $name)->where('filename', $filename);

        if (!$nameFilter->isEmpty()) {
            throw new \Exception('Already exists!');
        }

        $command = new Command($name, $filename, $data);
        $this->commandFiles->push($command);

        return $command;
    }

    public function viewCommand($name, $version)
    {
        foreach ($this->commandFiles as $commandFile) {
            if ($commandFile->name == $name && $commandFile->hasVersion($version)) {
                return $commandFile->render();
            }
        }
    }
}

Command Class

class Command
{
    protected $name;
    protected $filename;
    protected $version;

    protected $data;

    public function __construct($name, $filename, $data)
    {
        $this->name = $name;
        $this->filename = $filename;
        $this->data = $data;

        $this->version = collect();
    }

    public function forVersion($versions)
    {
        foreach ($versions as $version) {
            $this->version->push($version);
        }
    }

    public function render()
    {
        return view($this->filename, $this->data)->render();
    }

    public function hasVersion($number)
    {
        return $this->version->contains($number);
    }

    public function __get($property)
    {
        if (property_exists($this, $property)) {
            return $this->$property;
        }
    }
}
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

I would suggest that you instantiate your Command object and inject into CommandCollection via your addCommand() method.

$command = new Command('add-site', 'path/script_one.sh', $data);
$command->forVersion([4, 5]);
$commandCollection->addCommand($command);

This gives you clear separation of logic such that your collection no longer needs to understand how to instantiate the Command object. It also removes the obfuscated logic of calling forVersion() on a Command object returned from addCommand(). Why would addCommand() be expected to return a Command object instead of simply adding a Command object to the collection?

Passing the Command dependency like this would also allow you to type hint that a valid Command object is passed. Right now, you are doing absolutely nothing to validate that that the parameters being passed to this method (or really all other public methods across these two classes) contain good values to work with.


Should data passed to forVersion() be information that is actually passed to the Command constructor, as opposed to being passed in a separate method? Does the version data really need to be mutable on the object such that there should be a setter method like this? Are you allowing your Command objects to be instantiated into a bad or incomplete state?


Should you really be using Laravel Collection object as the basis for your collection class? In other words, do you really want to you an array for your data structure here? Note how in your methods you will have to repeatedly iterate over the collection/array to determine if certain entries are present, or to get their values. Your addCommand() method performs an \$O(n^2)\$ search against your underlying Collection just to be able to add an entry. Your viewCommand() requires an \$O(n)\$ iteration over the collection in order to return a Command file rendering.

This might not be a problem if you are only ever expecting to be working with a handful of entries in the collection, but will become a problem if you expect your collection to grow to significant sizes (even 100 entries would cause 10,000 individual iterations on the collection to be performed to add a new command, 1000 entries would require 1,000,000 iterations!).

If this concerns you, you might consider a key-value lookup scheme to allow for quick \$O(1)\$ lookup of results.

For example a structure like the following would enable fast lookup against the collection.

[
    '{filename}' => [
        {version} => {Command Object},
        {anotherversion} => {Command Object},
        ...
    ],
    '{anotherFilename}' => [ ... ],
    ...
]

With a viewCommand() method like:

public function viewCommand($name, $version)
{
    if(isset($commandFiles[$name][$version])) {
        return $commandFiles[$name][$version]->render();
    }
    return false;
}

Why should the collection hold a method for rendering the command? You already have the render() command living properly on the object, so isn't this basically a find() method?

For example:

$command = $collection->find($name, $version);
$command->render();

The collection should just be a collection. With methods only for managing/accessing the collection not for calling methods on the objects contained in the collection.

Note that generalizing to just a find() method would also allow you to call this method in the addCommand() method to see if command already exists.

Note that if you decided you still want to stick with Laravel Collection as your base data structure, then you might also get to the realization that your collection class really adds little value above and beyond what the Laravel class adds. So why not:

$collection = collect([]);
$collection->push(new Command(...));

//  and to render
$command = $collection->where('name', $name);
$command->render();

Otherwise you might find yourself eventually adding filter, sort, etc. methods to your class because you have hidden away the underlying Collection capabilities.


Is it conceivable that a command in the collection could be replaced with a new Command object? If so, then you should not be throwing exception in this case. I would not think a collection would necessarily need to hold the logic as to whether an item in the collection could be replaced or not, as a caller could easily check for existence of an item in the collection and make decision to overwrite or not.


public function __get($property)
{
    if (property_exists($this, $property)) {
        return $this->$property;
    }
}

What if property does not exist? Should you throw exception? This condition would likely mean caller is accessing this class in unexpected fashion and should probably fail loudly.


$commandFile->name == $name

Consider using exact comparisons as default behavior, only using loose comparisons where there truly is a use case for it.

\$\endgroup\$
3
  • \$\begingroup\$ Great answer, thank you. One thing I don't understand why did you put curly bracket for key name in an array? eg: '{filename}' => [ ] \$\endgroup\$ May 22, 2017 at 19:58
  • \$\begingroup\$ @I'll-Be-Back I would trying to indicate that this was just a placeholder for a real filename, version, etc. value, not as a literal data structure. \$\endgroup\$
    – Mike Brant
    May 22, 2017 at 20:11
  • \$\begingroup\$ @I'll-Be-Back I would trying to indicate that this was just a placeholder for a real filename, version, etc. value, not as a literal data structure. \$\endgroup\$
    – Mike Brant
    May 22, 2017 at 20:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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