SitePoint Sponsor

User Tag List

Results 1 to 5 of 5
  1. #1
    SitePoint Member
    Join Date
    Mar 2005
    Posts
    7
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    Arrow Please review my code

    Hello,

    Ive been programming in php for a few years now but never really took any notice of how practical or versatile my code is. Im currently working on building myself a portfolio site (something ive been meaning to for a long time). Please take a look at a couple of snippets of my code and give me some feedback.

    PHP Code:
    <?php

    /*********************************************************************************
     *       Filename: portfolioManager.lib.php
     *       Version 1.0
     *       Copyright 2007 - 2009 (c) George Palmer
     *       Last modified: 20 Dec 2008
     *********************************************************************************/

    require_once('../inc/lib/dbManager.lib.php');
    /*********************************************************************************/
        
    class article implements IteratorAggregate
        
    {
            protected 
    $article;
            
            public function 
    __construct($article NULL)
            {
                    
    $this->article $article;
            }
            
            public function 
    __get($property)
            {
                if(isset(
    $this->article[$property]))
                {
                    return 
    $this->article[$property];
                } else {
                    return 
    false;
                }
            }
            
            public function 
    __set($property$value)
            {    
                    
    $this->article[$property] = $value;
            }
            
            public function 
    getIterator()
            {
                return new 
    ArrayObject($this->article);

            }
        }
    /*********************************************************************************/
        
    class portfolio implements IteratorAggregate
        
    {
            protected 
    $articles;
            
            public function 
    __construct($articles NULL)
            {
                if(
    $articles != NULL)
                {
                    
    array_push($this->articles$articles);
                }
            }    
            
            public function 
    displayALL($template)
            {
                foreach (
    $this->getIterator() as $article)
                {
                    
    $articleT = new templater($template);
                    
    $articleT->build($article->getIterator());
                    
    $articleT->publish();
                }
            }
            
            public function 
    displayRand($template)
            {
                
    $key array_rand($this->articles);
                
    $article $this->articles[$key];
                
    $articleT = new templater($template);
                
    $articleT->build($article->getIterator());
                
    $articleT->publish();
            }
            
            public function 
    __set($articles NULLarticle $article)
            {
                
    $this->articles[] = $article;
            }
            
            public function 
    getIterator()
            {
                return new 
    ArrayObject($this->articles);
            }
        }
    /*********************************************************************************/

    ?>
    The article class is takes a list of properties and their values from the database row. This object is then stored in the portfolio object for each row.

    Depending on the page and result set the articles are displayed like so.

    PHP Code:
    /*********************************************************************************
     *       Filename: home.inc.php
     *       Version 1.0
     *       Copyright 2007 - 2009 (c) George Palmer
     *       Last modified: 21 Dec 2008
     *********************************************************************************/
        //header template
            
    $headerT = new templater('templates/header.tem.php');
                
    $headerV = array('TITLE' => page::getPageName(false),
                                 
    'HEAD'     => page::getPageName(true));
                
    $headerV = new ArrayObject($headerV);
            
    $headerT->build($headerV);
        
    //end header template
        
        //footer template
            
    $footerT = new templater('templates/footer.tem.php');
                
    $footerV = array('FOOT' => 'Your viewing the '.page::getPageName(true).' page on '.date("D jS, g:i a").'.');
                
    $footerV = new ArrayObject($footerV);
            
    $footerT->build($footerV);
        
    //end footer template
    /*********************************************************************************/
        //libarys required
            
    require_once('../inc/lib/portfolioManager.lib.php');
        
    //end libarys
    /*********************************************************************************/
        //start page
            
    $dbPortfolio = new portfolioConnect//connect to the portfolio database
            
    $articles $dbPortfolio->fetchAll();//fetch all articles
            
            
    $headerT->publish();//publish header
            
            
    $articles->displayRand('templates/articleSingle.tem.php');//display all articles using template
            
            
    $articles->displayAll('templates/articleAll.tem.php');//display all articles using template
            
            
    $footerT->publish();//publish footer
        //end page
    /*********************************************************************************/ 
    or

    PHP Code:
    /*********************************************************************************
     *       Filename: article.inc.php
     *       Version 1.0
     *       Copyright 2007 - 2009 (c) George Palmer
     *       Last modified: 21 Dec 2008
     *********************************************************************************/
        //libarys required
            
    require_once('../inc/lib/portfolioManager.lib.php');
        
    //end libarys
    /*********************************************************************************/
        //start page before header template in order to fetch title
            
    $articleID $_GET['a']; //unclean!!!!!!!!!!!
            
            
    $dbPortfolio = new portfolioConnect//connect to the portfolio database
            
    $article $dbPortfolio->fetchSingle($articleID);//fetch single article
        //part end page
    /*********************************************************************************/
        //header template
            
    $headerT = new templater('templates/header.tem.php');
                
    $headerV = array('TITLE' => 'George Palmer &raquo; Articles &raquo; '.$article->title,
                                 
    'HEAD'     => $article->title);
                
    $headerV = new ArrayObject($headerV);
            
    $headerT->build($headerV);
        
    //end header template
        
        //footer template
            
    $footerT = new templater('templates/footer.tem.php');
                
    $footerV = array('FOOT' => 'Your viewing the "'.$article->title.'" article on '.date("D jS, g:i a").'.');
                
    $footerV = new ArrayObject($footerV);
            
    $footerT->build($footerV);
        
    //end footer template
    /*********************************************************************************/
        //start page
            
            
    $headerT->publish();//publish header
            
            
    $articleT = new templater('templates/articleDetailed.tem.php');
            
    $articleT->build($article->getIterator());
            
    $articleT->publish();
            
            
    $footerT->publish();//publish footer
        //end page
    /*********************************************************************************/
    ?> 
    Im also looking for a way to implement paging into this, if anyone has any suggestions.

    Many Thanks
    George Palmer

  2. #2
    SitePoint Zealot
    Join Date
    May 2008
    Location
    Montreal
    Posts
    155
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Note: my first two comments aren't so much to do with the quality of your code, but more then conventions you follow.

    First, I find most of your comments get in the way of your code. The large '*' separator blocks are unnecessary, and there isn't much point to telling us that you're done working with a particular section, you may simply comment that you're starting a new section. Consider, however, whether or not you actually *need* to tell the person what your doing (assuming the code is unobvious). For example: "//publish header" is entirely unnecessary when the code it describes is "$headerT->publish();".

    Second, I find your naming scheme a bit strange: portfolioConnect isn't very descriptive of what the code does. Regardless, this is just a matter of personal taste.

    Finally, it's nice to see that you're using the PHP5 SPL classes where appropriate. I'm curious, do you need ArrayObject where you use it or do you just want to use arrays as objects?

  3. #3
    SitePoint Member
    Join Date
    Mar 2005
    Posts
    7
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Thanks Peter, good comment on the commenting, ive not really thought about whether its necessary or not. The way i comment is a habit i picked up from university, my lecture always said its better to over comment than not to comment enough. While this may be true in education in the real world i can see its not as practical.

    To be honest ive not really picked up a naming convention, although consistent in each application, ive not really found something that sticks yet. Would you have any advice of know of any text that may be of assistance?

    As for ArrayObject in most cases I need to use it, but I have used it unnecessarily at times because ive read that it’s a good habit to get into as it makes fewer internal copies compared to directly iterating over an array and therefore is faster.

  4. #4
    SitePoint Addict
    Join Date
    Nov 2005
    Location
    Germany
    Posts
    235
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Geo200k View Post
    Thanks Peter, good comment on the commenting, ive not really thought about whether its necessary or not. The way i comment is a habit i picked up from university, my lecture always said its better to over comment than not to comment enough. While this may be true in education in the real world i can see its not as practical.
    I recommend if you think you need to comment a few lines you should move those lines into a separate function with a descriptive name.

    Apart from that I avoid __get() and __set() because I feel more comfortable with explicit interfaces.

  5. #5
    Use The Cloud
    Join Date
    Jan 2006
    Location
    Boise, ID
    Posts
    556
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Pick a well known coding standard such as PEAR or Zend Framework.

    Both of these include documenting your code using the well known PHPDoc standard.
    Brad Hanson, Web Applications & Scalability Specialist
    ► Is your website outgrowing its current hosting solution?
    ► PM me for a FREE scalability consult!
    ► USA Based: Available by Phone, Skype, AIM, and E-mail.


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •