Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've written a custom PHP session class for handling sessions across the web app. Please review the code and point out mistakes and suggest better handling techniques.

require_once('config.php');

class Sessions {

protected $sessionID;

public function __construct(){
    if( !isset($_SESSION) ){
        $this->init_session();
    }
    //session_start();
    //$this->sessionID = session_id();
}

public function init_session(){
    session_start();
}

public function set_session_id(){
    //$this->start_session();
    $this->sessionID = session_id();
}

public function get_session_id(){
    return $this->sessionID;
}

public function session_exist( $session_name ){
    if( isset($_SESSION[$session_name]) ){
        return true;
    }
    else{
        return false;
    }
}

public function create_session( $session_name , $is_array = false ){
    if( !isset($_SESSION[$session_name])  ){
        if( $is_array == true ){
            $_SESSION[$session_name] = array();
        }
        else{
            $_SESSION[$session_name] = '';
        }
    }
}

public function insert_value( $session_name , array $data ){
    if( is_array($_SESSION[$session_name]) ){
        array_push( $_SESSION[$session_name], $data );
    }
}

public function display_session( $session_name ){
    echo '<pre>';print_r($_SESSION[$session_name]);echo '</pre>';
}

public function remove_session( $session_name = '' ){
    if( !empty($session_name) ){
        unset( $_SESSION[$session_name] );
    }
    else{
        unset($_SESSION);
        //session_unset();
        //session_destroy();
    }
}

public function get_session_data( $session_name ){
    return $_SESSION[$session_name];
}

public function set_session_data( $session_name , $data ){
    $_SESSION[$session_name] = $data;
}

}
share|improve this question
1  
Why is there so much core-functionality (at least by name) commented out? –  Vogel612 Aug 26 at 11:08
    
I don't understand your comment brother. Can you please explain? –  Syntax Error Aug 26 at 11:38
    
first off. I am not your brother. It's not really helpful and not winning you points with me. Secondly the core use of a session is starting it, right? why is $this->start_session() session_start() and the like commented out then?? –  Vogel612 Aug 26 at 11:40
1  
Oh right, actually I intended to use init_session() for starting session. And I've called it in the constructor so that the session starts automatically when the object is created. –  Syntax Error Aug 26 at 11:47
1  
a well meant "suggestion": don't use "bro", "brother", "dude" and the like if you intend to be polite. Usually it's fine to just drop the salutations, but if you really want to be polite, then "Sir", "Mr." and the like are the words of choice. Anyways, thanks for clarifying ;) –  Vogel612 Aug 26 at 12:07

2 Answers 2

up vote 5 down vote accepted

Commented out Code

You should remove code that is commented out. If you think that you might need it in the future, think about using version control.

set_session_id()

This is only called in your commented out code. Does the user have to call it manually? If they don't, get_session_id() will return a wrong result. Maybe rewrite it like this (you don't seem to be using the field $sessionID, so you might as well get rid of it):

public function get_session_id(){
    return session_id();
}

if-else and session_exist

If you have code like this:

if( isset($_SESSION[$session_name]) ){
    return true;
}
else{
    return false;
}

you can rewrite it like this:

return isset($_SESSION[$session_name]);

Also, when you define a function like this, then use it. Instead of

if( !isset($_SESSION[$session_name])  ){

write

if(!session_exist($session_name]) ){

XSS

Using Session for XSS might be a possibility, depending on how your code is used. So in display_session I would clean the session with htmlentities to prevent XSS attacks.

share|improve this answer
    
hey Tim thank you for pointing that our bro. I've fixed the code as per your suggestions. Can you suggest what more functions should I add in this class? Or what important functions am i missing. –  Syntax Error Aug 26 at 11:37
    
maybe remove_value? You could also look at what other people have in their session classes. codeigniter for example has Flashdata. But I think that you can just leave it as it is and add functionality as it is needed. –  tim Aug 26 at 11:45
    
This is only called in your commented out code. Does the user have to call it manually? No I've called init_session() in the constructor dude. –  Syntax Error Aug 26 at 11:49
    
Yes, but init_session() doesn't set $this->sessionID = session_id();. sessionID isn't set anywhere, and thus get_session_id() will always return null. –  tim Aug 26 at 11:51
    
yes yes that's right. I missed that, but now fixed it. Thanks for your help dude :) –  Syntax Error Aug 26 at 11:55

To be honest, it looks like a mess to me and it's difficult to read. Your naming convention is also off. They don't appear to do what you (or any other programmer) would think it does by looking at the name of your methods (not to mention your variables too).

  • Method session_exist() is not what it seems. It's not checking if a session exist, but it's rather checking if a session parameter exist. I would rename that to parameterExist().

  • Method create_session() does not create sessions. It sets session parameters. I would call it setParameter() instead.

  • Method insert_value(), what does it even do? :|

    So you're taking in a session name (?) and an array of data. Then you're checking if the session name is an array? And if it is you push the data array that's passed in to it? Is there any difference beween create_session() and insert_value()? I'm confused.

  • Why does your class contain a sessionID property?

    It looks like your using it to eh, differentiate between session objects? If that's the case the object itself can serve for that. Unless you have some other intentions for it I would just get rid of it and it's setter and getter.

I could go on and on, but this should be enough to let you realize other obvious problems that are an issue with your code.

share|improve this answer
    
session_exist() Originally I wrote this method with optional argument. Without the argument it would check that if session has been originated or not. I just forgot to change the name after having made the argument compulsory. create_session() Yes you are right about it, sir. insert_value() I'm using this method for inserting a new value in session array by using array push. create_session() just creates a new index in session array, on the other hand insert_value() inserts new value at a specified index of session array. sessionID I made for future use, but never used it. –  Syntax Error Aug 26 at 12:28
    
You can go on commenting on it, I'm learning by this way. and thanks alot for pointing these things out :) –  Syntax Error Aug 26 at 12:32

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.