My 'Base' class

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
shiznatix
DevNet Master
Posts: 2745
Joined: Tue Dec 28, 2004 5:57 pm
Location: Tallinn, Estonia
Contact:

My 'Base' class

Post by shiznatix »

I put together a simple style framework to make my life a bit easier and the biggest, most important part is the Base class. All of my classes extend the Base class which does most of the work. I would like to know if there is any room for improvement or anything thats just crap in this. It is a bit long so if you take the time to look at any of it I would be very grateful, i don't expect anyone to go though the whole damn thing. Oh and I commented it...JUST FOR YOU :lol: !!!

Code: Select all

<?php

/****************************************
*                                       *
*  Creator: --- ---               *
*  Contact: shiznatix AT gmail DOT com  *
*  Drink: Beefeater and Tonic           *
*                                       *
****************************************/

class Base
{
    public $db;         //Database class
    //public $Data;
    public $Extensions; //Extensions class
    public $Errors;     //Errors array
    public $View = '';  //Current View

    //private $Config;    //Configuration file to find other files

    /**
     * Just gets out db and extensions classes kiddies!
     * This function is not always called as it is never initiated or whatever
     */
    public function __construct()
    {
        global $db;
        $this->db =& $db;
        global $Extensions;
        $this->Extensions =& $Extensions;
    }

    /**
     * This function allows for getting a list from a database easily
     * So we can stay away from writing the same queries over and over.
     *
     * @param fields: is the fields from the databse we are selecting
     * @param conditions: is the "this = that" stuff
     * @param mode: how to join the conditions together
     * @param tail: the end of the query such as 'LIMIT' or whatever
     * @return array of rows from fetch_assoc()
    */
    public function GetList($fields = null, $conditions = null, $mode = 'AND', $tail = null)
    {
        $this->db->Select($this->Table, $fields, $conditions, $mode, $tail);
        return $this->db->GetDBVarsList(get_class($this));
    }

    /**
     * This function will get a list from a database easily
     * then set the values from the DB to the Model class variables
     * automatically.
     *
     * @param fields: is the fields from the databse we are selecting
     * @param conditions: is the "this = that" stuff
     * @param mode: how to join the conditions together
     * @param tail: the end of the query such as 'LIMIT' or whatever
     * @return true/false if it fails or not (she never fails to get the job done... OH!)
    */
    public function Load($conditions = null, $mode = 'AND', $tail = null)//$tail = ' LIMIT 1 '
    {
        $this->db->Select($this->Table, false, $conditions, $mode, $tail);
        $DBVars = $this->db->GetDBVars();

        if (empty($DBVars))
        {
            $Exempt = array('db', 'Data', 'Extensions', 'Errors', 'View', 'Config', 'Table');

            $ClassVars = get_class_vars(get_class($this));

            foreach ($ClassVars as $key => $val)
            {
                if (!in_array($key, $Exempt))
                {
                    $this->$key = '';
                }
            }

            return false;
        }

        $ClassVars = get_class_vars(get_class($this));

        foreach ($ClassVars as $key => $val)
        {
            if (array_key_exists($key, $DBVars))
            {
                $this->$key = $DBVars[$key];
            }
        }

        return true;
    }

    /**
     * Gets the count of affected or returned rows from
     * the last executed query
     *
     * ToDo('Add optional query resource to get information from previous queries');
     *
     * @return int count
    */
    public function GetCount()
    {
        return $this->db->GetCount();
    }

    /**
     * Gets the last inserted Id for that db connection
     *
     * @return int Id
    */
    public function GetLastInsertId()
    {
        return $this->db->GetLastInsertId();
    }

    /**
     * This class will include then load another class (Module)
     * All class objects are stored in the singleton ObjectStorage
     *
     * ToDo('Make sure that the class file exists before getting it');
     *
     * @param class is the name of the class to load (from the config file)
     * @param leve is the Business, Model, or View level
     *
     * @return newly created object
    */
    public function LoadClass($class, $level)
    {
        global $Config;
    
        require_once $Config[$class]['Path'].$class.'.'.$level.'.class.php';
    
        $ClassName = $class.$level;
    
        $obj = ObjectStorage::GetObject($ClassName);
    
        if (false !== $obj)
        {
            return $obj;
        }
        else
        {
            $obj = new $ClassName;
            ObjectStorage::AddObject($ClassName, $obj);
            return $obj;
        }
    }

    /**
     * This function will automatically set the values of the
     * class vars that call it to the $vars (usually $_POST or $_GET)
     *
     * @param vars the array of variables to set to the class variables
    */
    public function AutoSet($vars)
    {
        if (is_array($vars))
        {
            $ClassVars = get_class_vars(get_class($this));

            foreach ($vars as $key => $val)
            {
                if (array_key_exists($key, $ClassVars))
                {
                    $this->Set($key, $val);
                }
            }

            return true;
        }
        else
        {
            return false;
        }
    }

    /**
     * This function will set a single class variable to a value
     * that value can be any type of variable, even an array
     *
     * @param var is the variable in the class to be set
     * @param val is the value you want the variable to be
    */
    public function Set($var, $val)
    {
        $ClassVars = get_class_vars(get_class($this));

        if (array_key_exists($var, $ClassVars))
        {
            if (is_array($val))
            {
                foreach ($val as $key => $value)
                {
                    $val[$key] = trim($value);
                }
            }
            else
            {
                $val = trim($val);
            }

            $this->$var = $val;
            return true;
        }
        else
        {
            return false;
        }
    }

    /**
     * This function deletes a class variable
     *
     * @param var is the variable to delete
    */
    public function DelVar($var)
    {
        $ClassVars = get_class_vars(get_class($this));

        if (array_key_exists($var, $ClassVars))
        {
            unset($this->$var);
            return true;
        }
        else
        {
            return false;
        }
    }

    /**
     * This function sets the view to be outputted to the user
     * it gets the info to output from this modules View level class
     *
     * @param view the name of the function in the view level class to output
    */
    public function SetView($view)
    {
        $this->View = $view;
    }

    /**
     * This function gets the view to be shown then returns its output
     *
     * @return the output from the view function
    */
    public function ShowView()
    {
        if ('' === $this->View)
        {
            $this->View = 'Main';
        }

        $function = 'View'.$this->View;

        return $this->$function();
    }

    /**
     * This function deletes a row from the database. It requires
     * that this class have an Id given to it so it can delete only
     * a single row at a time from the database to avoid a really stupid
     * mistake.
     *
     * ToDo('be able to give an array of Ids to be deleted');
     *
     * @param mode is the mode the conditions will be joined together in
     * @param tail is the end of the SQL query if you need one
    */
    public function Remove($mode = 'AND', $tail = 'LIMIT 1')
    {
        if (empty($this->Id))
        {
            trigger_error('Id is required', E_USER_ERROR);
        }
        else
        {
            $conditions['Id'] = $this->Id;

            $this->db->Remove($this->Table, $conditions, $mode, $tail);
        }
    }

    /**
     * This function will save the class variables to the database.
     * If an Id exists, then it runs an UPDATE query, otherwise it runs
     * an INSERT query
    */
    public function Save()
    {
        $skip = array('Table', 'Data', 'Id', 'db', 'Extensions', 'Errors', 'View');

        if (empty($this->Id))
        {
            foreach ($this as $key => $val)
            {
                if (!in_array($key, $skip) && '' != $val)
                {
                    $fields[] = $key;
                    $values[] = $val;
                }
            }

            $this->db->Insert($this->Table, $fields, $values);
        }
        else
        {
            foreach ($this as $key => $val)
            {
                if (!in_array($key, $skip) && '' != $val)
                {
                    $set[$key] = $val;
                }
            }

            $this->db->Update($this->Table, $set, array('Id' => $this->Id));
        }
    }

    /**
     * This function gets the value of a class variable
     *
     * @return the value of the class variable
    */
    public function Get($var)
    {
        $ClassVars = get_class_vars(get_class($this));

        if (array_key_exists($var, $ClassVars))
        {
            return $this->$var;
        }
        else
        {
            trigger_error($var.' Does not exist', E_USER_ERROR);
        }
    }

    /**
     * This function will load an extension from the extensions class
     *
     * @param extension is the name of the extension to load
     * @param params is any parameters to pass to the extension to load with
     * @return extension instance
    */
    public function LoadExtension($extension, $params = false)
    {
        return $this->Extensions->$extension($params);
    }

    /**
     * This function will save an error message to be displayed and/or
     * delt with later on in execution
     *
     * @param message the error message you whish to show
     * @param field is the optional field to associate the error with
    */
    public function SetError($message, $field = false)
    {
        $this->Errors[] = $message;

        if (false !== $field)
        {
            $this->Errors['ErrorFields'][] = $field;
        }
    }

    /**
     * Returns the array of errors
    */
    public function GetErrors()
    {
        return $this->Errors;
    }

    /**
     * This function will get the errors from another object
     * and import them to its own error array
     *
     * @param object is the object whos errors you want
    */
    public function ImportErrors($Object)
    {
        if (!is_object($Object))
        {
            trigger_error('Object required but '.gettype($Object).' was given', E_USER_ERROR);
        }

        if (false !== $Object->isErrors())
        {
            foreach ($Object->Errors as $val)
            {
                $this->SetError($val);
            }
        }

        return true;
    }

    /**
     * This function will check for any set errors
     *
     * @return true if there are errors, false if there are none
    */
    public function isErrors()
    {
        if (is_array($this->Errors))
        {
            return true;
        }
        else
        {
            return false;
        }
    }

    /**
     * This function will redirect a user to another page with the option
     * of sending variables though the url with them
     *
     * @param location is the url to send them to
     * @param vars is the variable to put in the URL
    */
    public function Redirect($location, $vars = false)
    {
        $tail = '';

        if (false !== $vars)
        {
            if (is_array($vars))
            {
                $tail .= '?';

                foreach ($vars as $key => $val)
                {
                    $tail .= $key.'='.$val.'&';
                }

                $tail{strlen($tail)-1} = '';//remove the trailing & sign
            }
        }

        header('Location: '.$location.'.php'.$tail);
        die();
    }
    
    /**
     * This function will add anything extra to the header of the HTML document
    */
    public function AddHeaderHTML($Data)
    {
        $GLOBALS['HeadHTML'] .= $Data;
    }

    /**
     * This will check to see if a user is 'logged in' based on
     * the return value of function isLoggedIn() returns
     *
     * @param UserTypeId is the Id of the type of user that is allowed to access this page
    */
    public function LoginRequired($UserTypeId = false)
    {
        if (false === isLoggedIn())
        {
            $this->Redirect('index');
        }
        else
        {
            if (false !== $UserTypeId)
            {
                if ($UserTypeId !== GetUserType())
                {
                    $this->Redirect('index');
                }
                else
                {
                    return true;
                }
            }
            else
            {
                return true;
            }
        }
    }
}

//END!
?>
Last edited by shiznatix on Fri Mar 09, 2007 11:54 am, edited 1 time in total.
User avatar
John Cartwright
Site Admin
Posts: 11470
Joined: Tue Dec 23, 2003 2:10 am
Location: Toronto
Contact:

Post by John Cartwright »

After a very brief glance, here are my comments.
  • I don't really understand why all your classes would extend this monster class. It seems your trying to implement MVC. Typically, only the controller would extend your core application controller.
  • I don't understand why you store all your classes in a singleton. This is a major code smell.
  • Your use of globals is another code smell, you should typically be passing the objects around
  • Your application core class seems to be trying to do way to many things. You should at minimum consider separating this into individual objects
  • I don't really see a point of mixing in the database at this point. The model is responsible for data handling.
  • on code like return $this->$function(); , you might want to check the methods exist first.. is_callable() might be of interest.
  • I don't understand your usage of $this->Set(), $this->Get(), you are loading all the class variables into an array, and checking against the array, when simply you can use isset()
  • There is much more confusion, but I'll leave it at that
Perhaps you should show us the class in action.
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Post by Chris Corbyn »

Jcart wrote:I don't really understand why all your classes would extend this monster class. It seems your trying to implement MVC. Typically, only the controller would extend your core application controller.
Agreed. Although I can see what the objective is. I guess it provides "core functionality" to all the components in the app although I do prefer to have to know about the classes/object in my framework and what their roles are.

Jcart wrote:I don't understand why you store all your classes in a singleton. This is a major code smell.
Why? It would be if *all* objects were put in a singleton but using a registry is often a good thing to do. I've never generally stumbled across any problems with needlessly registering objects and this approach is actually a brilliant way to acheive one of your next point, if the object will be used by many other objects.
Jcart wrote:Your use of globals is another code smell, you should typically be passing the objects around
Registry? Unless it's just a quick pass from one object to another.
Jcart wrote:Your application core class seems to be trying to do way to many things. You should at minimum consider separating this into individual objects
Agreed. It's handy to have wrappers which help to pull things together purely for convenience but the underbelly of such convenience classes would usually call upon other relevant classes.
Jcart wrote:I don't really see a point of mixing in the database at this point. The model is responsible for data handling.
No issue with having a connection available if Model will also extend this. Although that does start to get a bit smelly. Duplicating methods which could be handled in the DB class itself is a little bit loosely structured though. I'd rather have all Model components extend a Base class which is targeted purely at the Model and provides the ability to just do: $this->db->execute(" ..... "); etc

I work with an application which uses multiple databases so we actually end up doing this:

Code: Select all

$rs = $this->db("app1")->execute(" .... ");
$rs2 = $this->db("app2")->execute(" ... ");
It feels quite natural to use composition to distinguish between layers in an application.
Jcart wrote:I don't understand your usage of $this->Set(), $this->Get(), you are loading all the class variables into an array, and checking against the array, when simply you can use isset()
I actually like doing things like this. If I'll constantly be accessing values in a container I usually abstract methods for get(), set(), remove() and has(). It makes it easier to interface with externally should the need arise at any point.
User avatar
shiznatix
DevNet Master
Posts: 2745
Joined: Tue Dec 28, 2004 5:57 pm
Location: Tallinn, Estonia
Contact:

Post by shiznatix »

I have taken everything into consideration and will update and repost when done. Thanks much.
User avatar
Maugrim_The_Reaper
DevNet Master
Posts: 2704
Joined: Tue Nov 02, 2004 5:43 am
Location: Ireland

Post by Maugrim_The_Reaper »

Everyone else had good points. So I'll just be a stickler. :)

If Base is not directly instantiated, should the constructor be protected rather than public?
User avatar
shiznatix
DevNet Master
Posts: 2745
Joined: Tue Dec 28, 2004 5:57 pm
Location: Tallinn, Estonia
Contact:

Post by shiznatix »

Ok so after sitting around thinking for a while I can only think of just making 3 seperate 'base' classes. 1 for the View level, 1 for the Controller, and 1 for the Model. I would basically just split up the Base class I have now to just give the functionality to where it is needed. The only problem with that now is that the Model and the Controller would share some functions and all would get really messy and gross.

So here it is. I have the MVC going well but I need some global methods and whatnot. What is the best way to do this?
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Post by Christopher »

shiznatix wrote:Ok so after sitting around thinking for a while I can only think of just making 3 separate 'base' classes. 1 for the View level, 1 for the Controller, and 1 for the Model. I would basically just split up the Base class I have now to just give the functionality to where it is needed. The only problem with that now is that the Model and the Controller would share some functions and all would get really messy and gross.

So here it is. I have the MVC going well but I need some global methods and whatnot. What is the best way to do this?
Probably a Registry to store Datasource layer objects -- that's pretty common these days.

Out of curiosity, how did you divide the methods it your Base class above in M, V, C classes? And what methods do the Model and Controller share?,
(#10850)
User avatar
shiznatix
DevNet Master
Posts: 2745
Joined: Tue Dec 28, 2004 5:57 pm
Location: Tallinn, Estonia
Contact:

Post by shiznatix »

arborint wrote: Probably a Registry to store Datasource layer objects -- that's pretty common these days.
Can you clarify / give some links that explain this?
arborint wrote: Out of curiosity, how did you divide the methods it your Base class above in M, V, C classes? And what methods do the Model and Controller share?
I didn't go and do it but it was basically going to be like:

View and Controller shared all the load extensions and classes and setting the view.

Controller had all the error setting and getting functions

Model had the set, get, load, save, remove, getlist, getcount, getlastid and those

thats just what I was thinking, I didnt actually do anything yet because I dont think that would be the best design unless it is and i am wrong which happens a lot.
User avatar
John Cartwright
Site Admin
Posts: 11470
Joined: Tue Dec 23, 2003 2:10 am
Location: Toronto
Contact:

Post by John Cartwright »

Maugrim_The_Reaper wrote:If Base is not directly instantiated, should the constructor be protected rather than public?
I would tend to agree.
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Post by Chris Corbyn »

Maugrim_The_Reaper wrote:If Base is not directly instantiated, should the constructor be protected rather than public?
Or just make it abstract. Making it abstract would probably be a good idea if you do break it down into more targeted classes since it's likely the subclasses you make from it will all (maybe with the exception of model) need some method implemented, such as render() for view and run() for controller.
User avatar
shiznatix
DevNet Master
Posts: 2745
Joined: Tue Dec 28, 2004 5:57 pm
Location: Tallinn, Estonia
Contact:

Post by shiznatix »

What happened when I made the __construct() protected:

I initiated the class that inherited the base class and it died saying that the base classes __construct() was protected and that it couldnt continue. That seamed very very strange to me since the __construct() was not called in the Base class but I dunno. I didn't look too deep into it but It was defiantly giving that error when I initiated the extended section of the class.

As for the breaking it down into 3 different base classes, is this bad? I dont quite understand what arborint was saying.
User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Post by Jenk »

did you overload the constructor? when protecting a parent classes constructor, you must overload it in the child class.

the same effect is met when using abstract, as d11wtq pointed out.

I only use one base class, for the moment, in my framework and it's based on the Zend_Controller_Action class found in said framework. The class itself is abstract, and I'm going to suggest you do the same for your base class, as on it's own, the class is effectively useless - i.e. it must be extended to be functional.
User avatar
Maugrim_The_Reaper
DevNet Master
Posts: 2704
Joined: Tue Nov 02, 2004 5:43 am
Location: Ireland

Post by Maugrim_The_Reaper »

Zend_Controller_Action is a good reference, because you can dynamically pass objects into it using a few simple calls. The way it works in the ZF is as an abstract class. You extend this with a custom controller to add application/invocation specific objects, and extend this custom controller again with the final Action subclass which handles workflow.
Post Reply