Page 1 of 1

Documentation Critque ( PHPDocumentor )

Posted: Sat Jul 29, 2006 1:19 pm
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
?>


Posted: Sat Jul 29, 2006 2:43 pm
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

Posted: Sat Jul 29, 2006 2:57 pm
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?

Posted: Sat Jul 29, 2006 3:55 pm
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

Posted: Sat Jul 29, 2006 5:10 pm
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.

Posted: Sat Jul 29, 2006 5:44 pm
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!

Posted: Sat Jul 29, 2006 5:50 pm
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.

Posted: Sat Jul 29, 2006 6:42 pm
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.

Posted: Sat Jul 29, 2006 6:55 pm
by Ambush Commander
Okay... but you really asked for it. :-P