2
\$\begingroup\$

I really need a review on the structure, and how things are done. I want to improve.

Command.class:

<?php

    /**
     * Command
     * Processing commands to the server, acting, receiving response.
     *
     * @author Jony <driptonethemes@gmail.com>
     */

    class Command extends PrepareCommand
    {
        /**
         * Client
         * The command processor
         * @object Client
         */

        private $client;

        /**
         * Annonymous constructor
         * Constructs the application.
         *
         * @param message   ; Object Client
         */

        public function __construct(Client $message)
        {
            $this->client = $message;

            if ($this->isUsingCommand())
            {
                $this->_prepare();
            }
        }

        /**
         * Protected _prepare
         * Preparing the command line.
         *
         * @return void
         */

        protected function _prepare() 
        {
            $this->_commander = $this->client;
            $this->_process();
            $this->processCommand();
        }

        /**
         * processCommand
         * Processing the given commmand, finds out
         * what command was used, and it's data.
         *
         * @var _commandLine    ; The used command name.
         * @var _commandActions ; The data that comes after the command
         * @return void
         */

        private function processCommand()
        {
            switch ($this->_commandLine[1])
            {
                case "ban":
                    echo "LOl you tried to ban " . $this->fullString();
                    break;

                case "yell":
                    echo $this->fullString();
                    break;
            }
        }

        /**
         * isUsingCommand
         * Is the Client using a command at all?
         * Checks if line starts with the command sign " / ".
         *
         * @return boolean
         */

        public function isUsingCommand() 
        {
            return (substr($this->client->getMessage(), 0, 1) === "/") ? true : false;
        }

        /**
         * fullString
         * Gets all of the array elements of _commandActions and
         * converts it to one big String
         *
         * @var message         ; The String message.
         * @var _commandActions ; The data that comes after the command.
         * @return String
         */

        private function fullString()
        {
            $message = "";
            for ($i = 1; $i < count($this->_commandActions); $i++)
            {
                $message .= " " . $this->_commandActions[$i];
            }
            return $message;
        }


    }

?>

PrepareCommand.class:

<?php

    abstract class PrepareCommand 
    {

        /**
         * Properties
         */

        private $command;
        protected $_commander;
        protected $_commandActions;
        protected $_commandLine;

        /**
         * Processing the command, and calling parse
         * to explore the commandline.
         *
         * @return void
         */

        protected function _process()
        {
            $this->command = $this->_commander->getMessage();
            $this->parse();
        }

        /**
         * Parsing the command ; Exploring it ; 
         * Getting the first command and actions.
         *
         * @var _commandActions     ; Holding all of the words, between the entered spaces.
         * @var _commandLine        ; Getting the entered command name after the " / " sign.
         * @return void
         */

        private function parse() 
        {
            $this->_commandActions = explode(" ", $this->command);
            $this->_commandLine = explode("/", $this->_commandActions[0]);
        }   

        /**
         * Abstract prapre
         *
         * Activating the class, and preparing the command
         * through the class.
         *
         * @return void
         */

        protected abstract function _prepare();

    }

?>

usage:

new Command(new Client($_POST['process'], date('time'), "guest"));
\$\endgroup\$
3
\$\begingroup\$
  1. Stop using underscore to indicate private/protected parts of a class.
  2. Do not put logic in the constructors
  3. It is pointless to have ... ? true : false; in isUsingCommand().
  4. Use implode() in fullString()
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Any reason why is using underscore bad? I've been using it and helps me nicely to separate private and public methods. \$\endgroup\$ – Dmitri Zaitsev Sep 14 '13 at 19:53
  • \$\begingroup\$ @DmitriZaitsev that's what private and protected keywords are for. The underscore notation is a throwback from PHP4, when there were no options to set visibility of class members. The practice itself comes from Hungarian notation. So, to sum it up - unless you also use $iCount and cPuppy, and iPrintable or are stuck in PHP4, you should not use underscore for this purpose. \$\endgroup\$ – tereško Sep 14 '13 at 20:32
  • \$\begingroup\$ The keywords like private are only used in declarations. Everywhere else the variable is alone. By putting underscore I can easily see this variable's visibility without searching for its declaration. The hungarian $iCount is different - coming back after X months to my code I will ask - what's the hell is the i? But if I see $_count even after X years, I (and many developers) will still have the right guess about what underscore means. So the 1st notation was obscuring but the 2nd is helpful. So if that is the only reason, I am not convinced :). \$\endgroup\$ – Dmitri Zaitsev Sep 14 '13 at 20:42
  • \$\begingroup\$ The i is for "integer". Look up the "hungarian notation". \$\endgroup\$ – tereško Sep 14 '13 at 21:06
  • \$\begingroup\$ Not unless there is another type beginning with i. Anyhow, my point was that using underscore is helpful and suggestive and so improves my code readability imho. That is why I'm surprised to hear it is bad but I am ready to learn if there is any serious reason. \$\endgroup\$ – Dmitri Zaitsev Sep 14 '13 at 21:31
0
\$\begingroup\$

Such comments are obvious and help very little to read the code:

    /**
     * Annonymous constructor
     * Constructs the application.

Uncle Bob in his seminal Book "Clean Code" recommends to replace comments (that are often imprecise and tend to lie once code changes) as much as possible by expressive names of your methods.

For instance call your method _prepareCommandLine instead of just _prepare, then you don't need to put in the comments. Moreover, should you decide to change this method, you much more likely to change its name to reflect the new function, but can easily forget to change the comment.


This is well-known anti-pattern:

for ($i = 1; $i < count($this->_commandActions); $i++)

This way the count function executes in every single loop. You can avoid this:

for ($i = 1, $max = count($this->_commandActions); $i < $max; $i++)

However, if your loop only concatenates array, using implode is even better as suggested by tereško.


Why underscore for protected but none for private?

    private $command;
    protected $_commander;

That would make more sense:

    private $__command;
    protected $_commander;

Again, the name says little about what's going on:

private function parse() 

These comments are again obvious and worse, have misprints:

    /**
     * Abstract prapre
     *

This type of line is way too concrete for an abstract class:

        $this->_commandLine = explode("/", $this->_commandActions[0]);

You may want to revise your abstractions :)

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Any comment why the downvote? \$\endgroup\$ – Dmitri Zaitsev Sep 15 '13 at 18:43

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.