PHP MVC Framework Class

Question

In an effort to learn as much as possible about best development practices and software architecture, I created my own MVC framework using PHP (github: https://github.com/alemmedia/miniMVC). I've found that this has resulted in phenomenal improvements in my programming skills, but I'm still limited by my own limited experience when it comes to reviewing my code.

I'd like to get your critique on one of my least favorite classes (I always feel like there is something off about them):

Here is my Session singleton I use to access/manage session data.

class Session{


/**
 * @var object Holds the single instance of Session
 */
private static $instance;


/**
 * @var string The session id of the current session
 */
protected $id;


/**
 * @var mixed The data to be held in session.
 */
public $data;


/**
 * __construct - Starts session
 *
 *  Establishes Session::data reference to $_SESSION superglobal array, and sets session id.
 *  Privately held an called only by Session::open for singleton functionality
 */
private function __construct(){
    session_start();
    $this -> id   = session_id();
    $this -> data =& $_SESSION;
}


/**
 * open - Creates singleton instance of Session object
 * 
 * @return object
 */
public static function open() {
    if ( !isset(self::$instance) )
        self::$instance = new self();
    return self::$instance;
}


/**
 * set - Recieves variables to set to $_SESSION array
 *
 * @param string $property   The property to set to the session
 * @param mixed  $value      The balue to set to the property
 * @param bool   $make_array If set to true the property will be an numeric array
 */
public static function set($property, $value = null, $make_array = false){
    if ( is_array( $property) ){
        foreach($property as $key => $single_property)
            Session::open() -> set($key, $single_property);
    }else{
        if ($make_array == false)
            Session::open() -> data[$property] = $value;
        else
            Session::open() -> data[$property][] = $value;
    }
}


/**
 * del - Recieves variables to delete in $_SESSION array
 *
 * @param string $property   The property to delete from the session
 */
public static function del($property){
    if ( is_array( $property) ){
        foreach($property as $key => $single_property)
            Session::open() -> del($key);
    }else
        unset(Session::open() -> data[$property]);
}


/**
 * get - Returns variables from $_SESSION array.
 * 
 * @param string $property   The property to retrieve from the session
 * @return mixed|bool        The retrieved property or false.
 */
public static function get($property){
    if ( isset( Session::open() -> data[$property] ) )
        return Session::open() -> data[$property];
    else 
        return false;
}


/**
 * get - Returns variables from $_SESSION array and subsequently deletes them.
 * 
 * @param string $property   The property to retrieve and delete from the session
 * @return mixed             The retrieved property.
 */
public static function getThenDel($property){
    $result = Session::open() -> get ($property);
    Session::open() -> del ($property);
    return $result;
}

}

Let me know if you've spotted any mistakes or have any suggestions. Also, if you've found any code in the GitHub repository to be sub-optimal, I'd be happy to hear your comments.

Answer 1

Use braces {}! Can't say this enough. PHP has made many mistakes, but allowing this syntax is by far the worst. Adding those braces only increases file size by 2 bits! And it increases readability so much!

if ( !isset(self::$instance) ) { self::$instance = new self(); }

I'd check out the magic setter and getter methods (__set() and __get() respectively). Instead of calling a set or get method manually you just pass the variable you want. So...

$sessObj->set('var', 'value');
$sessObj->set('array', 'value', TRUE);
var_dump($sessObj->get('var'));
var_dump($sessObj->get('array'));
//compared to
$sessObj->var = 'value';
$sessObj->array[] = 'value';
var_dump($sessObj->var);
var_dump($sessObj->array);

Why are you not using $this? I mean, I don't use static methods at all really so correct me if I'm wrong, but from what I understand you can still access the internal instance by just using $this. So replace all Session::open() with $this. Even if you can't I would just dump all those static methods just for that alone. Why is everything static? If its not necessary, and I can't imagine why it would be, it shouldn't be static.

$this->set($key, $single_property);

Because my suggestions would change this code so drastically I will finish the review here. Hope this helps!

Answer 2

I have a similar session manager class in my MVC framework, and while I have absolutely nothing against Singletons (I apply the pattern where it makes sense, e.g. factories); I never felt that it made sense to use the pattern with my SessionManager class. If your goal was to prevent the object instance from being copied/cloned (by design or accidentally) you could opt for a regular ole' class and use private final __clone(){} to prevent the object instance from being cloned.

Using $this->method() was more legible (for me) than using self::method() or SessionManager::method() especially where referencing/calling class members/methods was concerned.