Page 1 of 1

Example code for new job

Posted: Fri Mar 19, 2010 8:42 am
by marty pain
pickle | Please use [ code=php ], [ code=text ], etc tags where appropriate when posting code. Your post has been edited to reflect how we'd like it posted. Please read: :arrow: Posting Code in the Forums to learn how to do it too.


I was asked to write a simple but well written class as an example of my work for a potential job. I had a script that updated a number of MySQL databases with a list of SQL of statements which I decided would be a good thing to write properly to show them.

The class is shown below. Please let me know what you think as I would really like some professional feed back before I send it off.

One thing I will note is that I have not yet sub-classed Exception. This is already in the list to be done.

Thanks!

Code: Select all

<?php
    
    class dbUpdater {
 
        private $dbListLocation;
        private $commandListLocation;   
        private $dbList;
        private $commandList;
    
        public function __construct($DBLocation, $commandLocation) {
            // Set the lists to the given locations
            $this->setLists($DBLocation, $commandLocation);
        }
 
        /**
         * SetLists()
         * 
         * Sets the location of the database and command lists
         * If either of the locations is not send as a string an exception is thrown
         *
         * @access public
         * @param string $DBLocation
         * @param string $commandLocation
         *
         */
        public function setLists($DBLocation, $commandLocation) {
            if(is_string($DBLocation) && is_string($commandLocation)) {
                // Set location Vars for the database and command list
                $this->dbListLocation = $DBLocation;
                $this->commandListLocation = $commandLocation;
 
                // Set the database and command lists
                $this->createLists();
            }
            else {
                throw new Exception( "Class dbUpdater - Both List arguments must be of type string!");
            }
        }
 
        /**
         * CreateLists()
         *
         * Creates an array of databases to update and an array of commands to issue
         * based on the current dbList and commandList respectively.
         * If either of the files do not exist, and exception is thrown.
         *
         * @access private
         *
         */
        private function createLists() {
 
            if(file_exists($this->dbListLocation)) {
                // Pick up list of datbase names
 
                $this->dbList = file($this->dbListLocation);
            }
            else {
                throw new Exception("Class dbUpdater - Database list file ({$this->dbListLocation}) does not exist.");
            }
 
            
            if(file_exists($this->commandListLocation)) {
 
                // Pick up list of commands to issue on each DB
 
                $this->commandList = file($this->commandListLocation);
 
            }
            else {
                throw new Exception("Class dbUpdater - Database list file ({$this->commandListLocation}) does not exist.");
            }
        }
        
        /**
         * updateDBs()
         *
         * Issue the commands stored in $commandList on the databases stored in $dbList
         * The method will thrown an exception if the it cannot connect to the speicifed database
         * or if an SQL command cannot be issued to the specified database
         *
         * @access public
         * @param resource $db
         *
         */
        public function updateDBs($dbc) {
            if(!is_resource($dbc)) {
                throw new Exception("Class dbUpdater - Database resource is not valid");
            }
 
            foreach($this->dbList as $database) {
 
 
                // Connect to database
                if(!empty($database)) {
 
                    $database = trim($database);
 
                    $con = mysqli_select_db($dbc, $database);
 
                    if(!$con) {
                        throw new Exception("Class dbUpdater - Unable to connect to database '$database'");
                    }
 
 
                    // Issue SQL                
 
                    foreach($this->commandList as $query) {
                        if(!empty($query)) {
 
                            $result = mysqli_query($dbc, $query);
                            if(!$result) {
                                throw new Exception("Class dbUpdater - Unable to execute query ($query) on database '$database'");
                            }
                        }
 
                    }
                }
 
            }
        }
 
        /** 
         * getDBListLocation()
         * 
         * Returns the location of the database list as a string
         *
         * @access public
         *
         */
        public function getDBListLocation() {
            return $this->dbListLocation;
        }
 
        /** 
         * getCommandListLocation()
         * 
         * Returns the location of the command list as a string
         *
         * @access public
         *
         */
        public function getCommandListLocation() {
            return $this->commandListLocation;
        }
 
        /** 
         * getDBList()
         * 
         * Returns the database list as an array
         *
         * @access public
         *
         */
        public function getDBList() {
            return $this->dbList;
        }
 
        /** 
         * getCommandList()
         * 
         * Returns the SQL command list as an array
         *
         * @access public
         *
         */
        public function getCommandList() {
            return $this->commandList;
        }
    }
?>

pickle | Please use [ code=php ], [ code=text ], etc tags where appropriate when posting code. Your post has been edited to reflect how we'd like it posted. Please read: :arrow: Posting Code in the Forums to learn how to do it too.

Re: Example code for new job

Posted: Fri Mar 19, 2010 9:36 am
by papa

Code: Select all

 
        /**
         * getCommandList()
         *
         * Returns the SQL command list as an array
         *
         * @access public
         *
         */
        public function getCommandList() {
            return $this->commandList;
        }
 
Hi,

There are people here that are much more qualified than me to comment your code. However I'm not a big fan of all the big comment blocks. It's easy enough to see if a function is public or not by looking at it for example.

Re: Example code for new job

Posted: Fri Mar 19, 2010 9:59 am
by marty pain
I'm not a big fan either, but I was told to add PHPDoc comments. I have just realised that I've missed off @throws though.

Re: Example code for new job

Posted: Tue Mar 23, 2010 9:10 am
by marty pain
Any more thoughts on this? I could really use the feed back!

Perfect or Pants? Let me know!

Re: Example code for new job

Posted: Fri Mar 26, 2010 9:52 pm
by zerkig
Not really qualified to comment but quick thoughts are

toString()
__destruct() method? overkill maybe?
instance() method for singleton implementation? really overkill

The __construct takes a filename and uses it on faith. Could that be susceptible to Path Traversal Attacks? However I'm probably paranoid whitelist check or sanitiser filter should be done at input.

Could you extend another class and implement an interface? I know it's pointless in this example but for a job interview showing you can implement others code might be important. Maybe pick one of the zend, solar, etc framework classes to extend. You'll have to weigh up the timescales of unit testing / documenting. There's a thought add a unit test with the code.

Another thought, the code is procedural in style e.g mysqli_query is used instead of mysqli::query notation so be prepared to answer a question on the advantages and disadvantages of that.

Why is it necessary to load the command list/ db list from file? Would it not be better to have a smaller class that you construct the command list one at a time with an addCommand method and a separate addDB method? What are the advantages of using mysqli_select_db? Is the advantage gained outweighed by the loss in reusabilty?

Re: Example code for new job

Posted: Mon Mar 29, 2010 8:45 am
by marty pain
Thanks Zerkig,

toString and __destruct did seem a little overkill. They wanted a simple class that worked well, so I didn't add any padding. Same goes for the lack of inheritance and interfaces.

__construct takes the file locations and passes them to setLists() that checks they are strings, then when the information is read in from file (in createLists()), it checks that the file exists. This wasn't done in the constructor as I wanted it to be flexible so you could update any number of databases on any number of servers by re-assigning the database locations and commands using setLists(). I use mysqli_select_db() so that the database on the server can be changed. This class was written specifically for MySQL databases.

Thanks for the notes on the use of either object or procedural style MySQL commands. I hadn't really though of it to be honest and just use the procedural style by default. I'll do some reading so I have a good answer if asked.

Thanks again mate!

Re: Example code for new job

Posted: Wed May 19, 2010 12:45 pm
by Pyrite
I have written phpDoc for several companies, that being said, I'll offer a few pieces of advice.

Firstly, you seem to have most of your methods phpDoc'ed. Sorry, do you want me to say functions? Ha. But what about your constructor? It equally is important, Also what about your member variables? They too need phpDoc. Lastly, the class itself should have a comment block. Usually in the class block I supply things like @package, @author, @version, @since and a description of what the class does and can do. Members generally have a one liner description above them like so:

Code: Select all

/** The command list location which is ...(and explain what it is) */
protected $commandListLocation;
Secondly, you are putting the names of the functions on the first line of the phpDoc block. Why? When you run phpDoc to generate your API documentation, the first line is the most important and should contain a short one-liner description of what the method/function does, followed by a period or a newline (like in your case). Two lines after that, it is typical to have a much larger description spanning more than 1 line and perhaps some example usage. Besides, the name of the function itself will already show up in your API Documentation without putting it explicitly on the first line of your phpDoc block.

Thirdly, in the method/functions, use of @param and @return tags are mostly key. I'm not positive right off the bat, but I think the access specifiers are not necessary, I think phpDoc will detect that from the use of public/private/protected already. What you aren't doing with your @param tags though is putting a description of them after the variable name. Typically you would put something like: @param array $arrFruit An array of fruit to convert into vegetables. Most of your function does not make use of the @return tag, which should explain what is returned, if anything.

In conclusion, I suggest you run phpdoc against your class and see how it produces the API Documentation, what it does and doesn't have. Remember it is not important to document how a method/function works. People don't care. It is only important to document how to use it. This is especially important in a company where others will be using your classes, they just need to know how to use it in their projects, not how you solved the algebra equation :D

Hope this helps.