Documentation Critque ( PHPDocumentor )

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
User avatar
neophyte
DevNet Resident
Posts: 1537
Joined: Tue Jan 20, 2004 4:58 pm
Location: Minnesota

Documentation Critque ( PHPDocumentor )

Post by neophyte »

Hey all I'm trying to learn PHPDocumentor today. I've written some. Please critque. I'm wondering did I use the tags right? Are there tags I could use better and are my comments useful?

Thanks!

Code: Select all

<?php
/**
* Gallery Controller for GOOP Gallery ver 2.0
*
* This is the contolling class for GOOP Gallery. It process requests and
* instantiates the "view" or module that is requested. Communication 
* between the two main classes dynamic_image and dynamic_gallery are done 
* through the controller.
*
* Copyright (C) 2004 - 2006 by Eugene Ritter
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
* 
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
*
* _________________________________________________________________________
*
* GOOP Gallery is available at http://www.webgeneius.com
* Comments & suggestions: http://www.webgeneius.com/contact.php
* With special thanks to devnetwork.net for all the support they've
* provided in my efforts to master PHP.
*
* @author Eugene Ritter
* @version 2.0.1
* @package goopgallery
*/

class gallery_controller {
/**
* The configuration variable for this class.
* Loaded from gallery_config.php
* @access private
*/
    var $CFG;
/**
* The path to various gallery modules
* @access private
**/
    var $module_path;
/**
* Holds the name of the gallery requested
* @access private
*/
    var $gallery;
/**
* Holds the request string variables
* @access private
*/
    var $request;


/**
* Constructor sets up $con
* @access public
* @uses module_config.php
* @uses dynamic_files.php
* @uses gallery_config.php
*/
    	function gallery_controller(){
		require_once('gallery_config.php');
		if(isset($CFG)){
			$this->set_global($CFG,'CFG');
		}
        	$this->CFG = &$CFG;
        	$this->module_path = $CFG->absolute_path . "modules/";
        	$this->get_request();
		$this->set_global($this->request, 'request');
        	$this->CFG->module = (!empty($this->CFG->module) && (!strcasecmp($this->CFG->module, 'default') == 0) && $this->module_exists($this->CFG->module))? $this->CFG->module : 'table';
       		require_once($this->module_path . $this->CFG->module . "/module_config.php");
		require_once($CFG->absolute_path.'lib/dynamic_files.php');
		$df = new dynamic_files();
		$this->set_global($df, 'df' );
	}
/**
* Adds objects and variables to $GLOBALS
* @access public
* @param object $object the object to be inserted into the $GLOBALS container
* @param string $handle optional name of class to be made globally accessible
*/	
	function set_global( &$object, $handle = '' ){
		$class = (empty($handle))? get_class($object): $handle;
		$GLOBALS[$class] = &$object;
	}
/**
* A "getter" for retrieving objects
* @param string $handle The name of the class to retrieve from the $GLOBALS array
*/
	function get_global($handle){
		return $GLOBALS[$handle];
	}
/**
* Calculate all of the request variables and stores them in the request property.
* Sets them equal to empty/null even if they're not used.
* Request contains the following default properties: id, gallery, image, large, page
* The module requested is stored in the $CFG.
* @access public
* @param array $additional The names of additional request names to be added to the $request object
*/
    function get_request($additonal=array()){
        $this->request->id = (isset($_REQUEST['id']))? intval($_REQUEST['id']) : '';
        // gallery
	$this->request->gallery = (isset($_REQUEST['gallery']))? urldecode($_REQUEST['gallery']): '';
        // image
	$this->request->image = (isset($_REQUEST['image']))? urldecode($_REQUEST['image']): '';
        // large
	$this->request->large = (isset($_REQUEST['large']))? $_REQUEST['large']: '';
        // page
        $this->request->page = (isset($_GET['page']))? intval($_REQUEST['page']): '';
        // module
        $this->request_module();
        //If you don't like the preset query values make you're own!
        if(!empty($additional)){
		foreach ($additional as $name => $value ){
			$this->request->$name = urldecode($value);	
		}		
        }
    }
/**
* This function calculates what module has been requested.
* $_POST methods override $_GET methods.
* $_COOKIES are also set and used to determine the requested module
* @access private
*/
    function request_module(){
        if (isset($_GET['module'] || $_POST['module'] )) {
		$module = ( $_POST['module'] ) ? $_POST['module'] : $_GET['module'];
            	if ($this->module_exists($module)) {
                	$this->set_module_cookie($module);
                	$this->CFG->module = $module;
            	}
        } else {
            	if (isset($_COOKIE['module'])) {
                	if ($this->module_exists($_COOKIE['module'])) {
                    		$this->CFG->module = $_COOKIE['module'];
                	}
            	}
        }
    }
/**
* Sets a cookie named module that holds the name of the module requested.
* @access private
* @param string $module contains the name of the module requested.
*/
    function set_module_cookie($module){
        setcookie('module', $module, time() + 60 * 60 * 24 * 30, '/', $this->CFG->domain);
    }
/**
* See if the module requested is the real thing or not.
* @access private
* @param string $module The name of the module to be tested.
* @return boolean
*/
    function module_exists($module){
        return is_dir($this->module_path . $module);
    }
/**
* This is an interface class. All modules are required to have a gallery_maker() method.
* Selects the modules, includes their code and instantiates them.
* The request logic for the default module 'table' is stored here.
* Request logic for other modules are contained in a requried method called 'gallery maker'
* @access public
* @uses dynamic_gallery.php
*/
    function gallery_maker(){
        require_once('dynamic_gallery.php');
        // Default table module load the gallery and handle requests
        if ($this->CFG->module == 'default' || $this->CFG->module == 'table') {
            // Load up the default gallery class.
            $this->gallery = new dynamic_gallery();
            if (($this->request->page != null || $this->request->gallery != null) && $this->request->large == null) {
                // prepare first view
                if ($this->gallery->get_files()) {
                    $this->gallery->first_view();
                }
            } elseif ($this->request->large != null) {
                $this->gallery->second_view($this->request->image);

            } else {
                $this->gallery->default_view();   
            }
            $this->gallery->output_gallery();
        } else {
            include($this->module_path . $this->CFG->module . '/module_gallery.php');
            $this->gallery = new module_gallery($this->request, $this->CFG);
            // If its a module request logic is stored
            // inside the module class in a function called
            // gallery_maker
            $this->gallery->gallery_maker();
        }
    }
/**
* Get file size, demensions and other information about an image
* @access public
* @param string $path Path to the image to get information about
* @param string $file The file name to get information about
*/
    function get_image_info($path, $file){
        require_once('dynamic_image.php');
        if (!isset($dynamic_image)) {
            $dynamic_image = new dynamic_thumbs($this->CFG);
        }
        return $dynamic_image->get_image_info($path, $file);
    }
/**
* Instantiates dynamic_thumbs
* @access public
* @uses dynamic_image.php
* @param string  $dir The path to the image
* @param string  $file The name of the image
* @param boolean $large Create a large image true or false?
* @param boolean $save  Save the file into a directory?
*/
    function make_images($dir, $file, $large = false, $save = false){
        require_once('dynamic_image.php');
        if (!isset($dynamic_image)) {
            $dynamic_image = new dynamic_thumbs($this->CFG);
        }

        if ($large === false) {
            $dynamic_image->set_thumbs($dir, $file, $save);
        } else {
            $dynamic_image->get_image_info($dir, $file);

            $dynamic_image->create_image(true);
        }
    }
} //end class
?>

User avatar
Nathaniel
Forum Contributor
Posts: 396
Joined: Wed Aug 31, 2005 5:58 pm
Location: Arkansas, USA

Post by Nathaniel »

The comments are good.

What you should refactor some is the code itself. Many times, comments are just fragrance for a code smell. Glancing over your class, I notice a couple of places where the code would be a lot cleaner if it was in its own class. It's much easier to have too few classes than too many, you know. :)

If you'd like a starting place, extracting the get_request method looks like a nice place to gain readability without too much work. Try porting that into a gallery_request class or similar.

Hope that helps :)

- Nathaniel
User avatar
neophyte
DevNet Resident
Posts: 1537
Joined: Tue Jan 20, 2004 4:58 pm
Location: Minnesota

Post by neophyte »

Interesting Ideas. I thnk it could be split into at least two maybe three:

request class
globals/registry class
and possibly
image request class

What say ye?
User avatar
Nathaniel
Forum Contributor
Posts: 396
Joined: Wed Aug 31, 2005 5:58 pm
Location: Arkansas, USA

Post by Nathaniel »

Hmm. I'm not quite sure what you mean with the image request class, but your first two sound solid. Take them one at a time and port them out of your controller class, and then see what is screaming to be extracted next. It very well may be an "image request" class.

:D
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

I actually use Doxygen, but I think I can still comment on documentation style.

Personally, when I document classes, I keep the comments with the same indent as the code, as well as indent the asterisks:

Code: Select all

/**
 * Summary
 * 
 * ...
 */
In terms of the documentation, I would advocate using the summary feature of PHPDocumentor: first line, describe the method as tersely as possible without repeating what can be determined from the function name, then go into detail. Furthermore, I wouldn't put the licensing information in the documentation: a developer trying to find out something about the API won't appreciate the GPL notice plastered over the page.

Make sure you compile your documentation. Otherwise, you have no idea whether or not it worked ;-)

Last I checked, PHPdoc style paragraphs need a double-space in order to be demarcated:

Code: Select all

/**
 * Summary
 * 
 * Par 1
 * Not par 2
 */

/**
 * Summary
 * 
 * Par 1
 * 
 * Par 2
 */
I completely agree with Nathaniel: refactoring also would be a good idea.
User avatar
neophyte
DevNet Resident
Posts: 1537
Joined: Tue Jan 20, 2004 4:58 pm
Location: Minnesota

Post by neophyte »

Great tips ambush commander. Will do.
Can't you do a simple @link to the GPL license?

Nathaniel ideas started me down a complete refactor project. For the first time, I'm putting the methods and properties and classes down on "paper" first. That way I can:

focus on naming things to carry more/better meaning
avoid duplicate or partially duplicate methods (not many of those)
identify and hopefully apply common coding patterns
right more efficient code

Hopefully I'll do better

BTW Who ever had the idea to create a critque forum -> GREAT IDEA!
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

Can't you do a simple @link to the GPL license?
I think a LICENSE file included in the tarball will suffice. @link doesn't say anything about licensing, it just says "Link!"
focus on naming things to carry more/better meaning
avoid duplicate or partially duplicate methods (not many of those)
identify and hopefully apply common coding patterns
right more efficient code
Good goals, although remember that efficiency/optimization can go against abstraction/readability.
Who ever had the idea to create a critque forum -> GREAT IDEA!
The only trouble is if it gets swamped with too many requests. I'm actually implementing the last phase of functionality for a library that I'm going to be releasing, and this is one of the first places I want to post the code.
User avatar
neophyte
DevNet Resident
Posts: 1537
Joined: Tue Jan 20, 2004 4:58 pm
Location: Minnesota

Post by neophyte »

Post it up! Although code reviews are time consuming I'm finding it worth it. I think there is a lot to be learned from productive critique of code. Although you might find more response from posting in segments whether than multiple classes and files all at once.
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

Okay... but you really asked for it. :-P
Post Reply