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 am building a PHP framework and would like to get some feedback on a few different sections of the project so far. I consider myself still a neophyte in PHP so I would like to ask if I'm going about completing these different tasks in an efficient and or correct way.

I am currently working on a session management class. What I have done is create methods that deal with a specific action when dealing with sessions. Pinoniq has suggested to me:

Always think this: everything you code must solve some kind of problem. If it doesn't solve a problem. Remove it.

So I kept that in mind when creating this class.

What I am trying to do in this class is create methods that do most of the work for you when creating and working with sessions.

Methods like namePattern() and ParamBreak() are operations for use inside the class class method.

class Sessions {
/**
 * Start a session
 * 
 * @param $id string - Set sestion id
 * @return boolean or trigger_error
 */
public function StartSession($id = null){
    if(!session_start()){
        trigger_error('Unble to create session',E_USER_WARNING);
    }
    if($id != null && is_string($id)){
        //Check pattern to make sure it's supported
        if($this->namePattern($id)){
            return session_id($id);
        }
    }
    return true;
}
/**
 * Create a session value in the $_SESSION array
 * if key is not supplied then one will be
     * created and incremented
 * automaticly 
 * 
 * @param $key string - Index key for array
 * @param $value mixed - value to be stored
 */
public function CreateSession($key = null,$value = null){
    if(!$key && !$value){
        trigger_error('Missing argument for Sessions::CreateSession()');
    }
    if(empty($value)){
        $value = $key;
        $key = null;
    }
    $_SESSION[$this->FindSessionKey($key)] = $value;
    return true;
}
/**
 * Return or set the session_id
 * 
 * @param $id string - string to set the id to
 * @return current session_id
 */
public function SessionId($id = null){
    if(isset($id)){
        return session_id($id);
    }
    return session_id();

}
/**
 * Check if the key supplied exists in the $_SESSION array
 * if not increment to the next value
 * 
 * 
 * @param $key mixed - 
 */
private function FindSessionKey($key){
    if(!empty($_SESSION)){
        $index = count($_SESSION)-1;
        if(!isset($key)){
            return $index+1;
        }else{
            if(array_key_exists($key,$_SESSION)){
                return $index+1;
            }else{
                return $key;
            }
        }
    }
    if(isset($key)){
        return $key;
    }   
    return 0;
}

public function SearchSessions($key = null,$value = null){

}
/**
 * Remove a session using a key as the needle
 * 
 * @param $key mixed - key to find the value
 * @return void
 */
public function RemoveSessionByKey($key){
    if(is_array($key) || ($key = $this->ParamBreak($key))){
        for ($i=0; $i < count($key); $i++) {
            unset($_SESSION[$this->KeyCycle($key[$i])]);
        }
    }else{
        unset($_SESSION[$this->KeyCycle(func_get_arg(0))]);
    }
}
/**
 * Find a return a key value
 * 
 * @param $key mixed - key to compare to
 * @return $key found
 * 
 * To be honest looking at this again I
 * have no idea whats going on. It works
 * but "WHAT WAS I DOING?" I don't know.
 */
public function KeyCycle($key){
    if(is_string($key)){
        $key = trim($key);
    }
    while(current($_SESSION) !== false){
        if($key == ($index = key($_SESSION))){
            //Prevent the removing of index 0
            //if $key index are not found
            if($key == 0 && $index == $key){
                return $index;
            }
            return $index;
        }
        next($_SESSION);
    }
}
/**
 * Remove a unset a session value using its
 * value as the needle
 * 
 * @param $value string - 
 */
public function RemoveSessionByValue($value){
    if(is_array($value) || ($value = $this->ParamBreak($value))){
        for ($i=0; $i < count($value); $i++){
            for($j=0; $j < count($_SESSION); $j++){
                unset($_SESSION[$this->ValueCycle($value[$i])]);
            }   
        }
    }else{
        for($j=0; $j < count($_SESSION); $j++){
            unset($_SESSION[$this->ValueCycle(func_get_arg(0))]);
        }
    }
}

protected function ValueCycle($value){
    if(is_string($value)){
        $value = trim($value);
    }
    while(current($_SESSION) !== false){
        if($value === current($_SESSION)){
            return key($_SESSION);
        }
        next($_SESSION);
    }
}

public function RemoveSessionByValuei($pattern){
    for ($i=0; $i < count($_SESSION) ; $i++) {
        if(is_array($_SESSION[$i])){
            for ($j=0; $j < count($_SESSION[$i]); $j++) {
                if(preg_match($pattern,$_SESSION[$i][$j],$match) > 0){
                    unset($_SESSION[$i][$j]);
                    $this->RemoveSessionByValue($match[0]);
                }
            }
        }else{
            if(preg_match($pattern,$_SESSION[$i],$match) > 0){
                unset($_SESSION[$i]);
            }
        }
    }
}

public function ReturnSession($key){
    if(array_key_exists($key, $_SESSION)){
        return $_SESSION[$key];
    }
    return false;
}

public function ReturnAllSessions(){
    if(!empty($_SESSION)){
        return $_SESSION;
    }
    return false;
}

public function ShowSessions(){
    return var_dump($_SESSION);
}

public function DestroySessions(){
    return session_destroy();
}

protected function ParamBreak($string){
    if(strpos($string,',') !== false){          
        return explode(',', $string);
    }
    return false;
}

private function namePattern($string){
    if(preg_match('/^([a-zA-Z0-9\,\-])+$/',$string) > 0){
        return $string;
    }
    return false;
}

}
share|improve this question
1  
Post rolled back. Please do not update code from answers; that will invalidate them. You may post a new follow-up question if you'd like further review. –  Jamal Apr 7 at 9:35
    
Thank you, understood –  andrewnite Apr 7 at 9:37
    
Thank you. For future reference, you don't need a "this post has been edited" anywhere in the post. This is all already documented below the post. –  Jamal Apr 7 at 9:39
add comment

2 Answers

First off the code is hard to make sense of, because I only see part of it. Most importantly I don't have the source for SessionsStruction class, which you are extending, and the name "Struction" doesn't make any sense to me (is it even in dictionary?).

It would also help to have a code sample of using this class.

The class also has no documentation describing its purpose - and after reading it all, I'm still wondering what's this all about.

Some immediate problems I see with this class:

  • It has too many responsibilities - parsing config file and debugging the sessions data (in ShowSessions method) should be handled outside of this class.

  • It has too wide API - an entire 13 public methods. Do you really need to provide such fine-grained interface?

  • It depends on global state: APP_PATH, CURRENT_FILE are defined somewhere else. Consider passing them in as parameters instead. Also the $_SESSION var.

  • why the empty __destroy() and SearchSessions() methods?

  • why the RemoveSession() which just delegates to RemoveSessionByKey()?

  • why the unused $active and $maltivalue class variables?

  • you really should check your spelling.

  • -
share|improve this answer
    
Thank you for your feedback. I have added more to the post in hope that it will help. I have removed the portions of the class that are not needed to explain and run the class. –  andrewnite Apr 7 at 6:11
    
Now that I've seen some examples of using this class, it makes me wonder, why should one use this whole complicated thing instead of directly using the $_SESSION variable? All the features it adds, look kinda weird and not very useful. Like when I ask it to create a field with already existing name, it assigns a numeric field instead, but I have no way of knowing, which field did it assign to. Why would I ever want that? –  Rene Saarsoo Apr 7 at 21:41
    
I understand, thank you for your comment. I am building a PHP framework to learn and experiment with PHP. I am currently working on the Session handling portion of it. I am posting my code to get feedback on its structure and code layout in hope to get any suggestions or advice on how it might be better. –  andrewnite Apr 8 at 3:29
add comment

Here is some examples in how this class can be used

$session = new Session();
//Start new session
$session->StartSession();

//Print sessions  ID
echo "SESSION ID: ".$session->SessionID();

$array = array(1=>'One',2=>'two',3=>'three', 4=>4);

 //Add value to session array
$session->CreateSession('Array',$array);
$session->CreateSession(1,'Zim');
$session->CreateSession('Tak');
$session->CreateSession('Gaz');

//.Uset session with ID 1
$session->RemoveSessionByKey(1);

//return value $array
$value = $session->ReturnSession('Array'); 

//Destroy session
$session->DestroySessions();


echo '<pre>';
echo $session->ShowSessions(); 
echo '</pre>';
share|improve this answer
add comment

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.