2
\$\begingroup\$

I have the following object oriented class revolving about the PHP super global $_SESSION I shown the script to a fellow developer and he mentioned that all my returns are not necessary needed, whereas I believe that they are?

My code is as followed:

/*
*
*
*   This class will allow control of the super global $_SESSION within PHP
*       It's possible to import this class within websites which are already working with session variables. 
*   Currently the functionality provides:
*       Invoking a new session
*       Getting the current session stats
*       Setting a session key/value
*       Getting the session value
*       displaying the session array
*
*
*
*/

class Session { 
    protected $Session_Started = false; 

    public function __construct(){ 
        /*
        *
        *   If session is already invoked,  the constructor
        *   will switch the protected variable to true, and the session_start function will not be called
        *       If session is not already invoked, the protected variable will remain false and provide the ability 
        *           To invoke a session_start() by calling $Class->init();
        */
            if (session_status() === 1){
                $this->Session_Started = true;
                return false; 
            }
            return true;
    }
    public function init(){
        /*
            Invoke a session

        */
        if ($this->Session_Started === false){
            session_start();
            $this->Session_Started = true;
            return true;
        }
        return false;
    }
    public function Status_Session(){
        /*
            Getting the current session status, and return readable information on the current status
        */
        $Return_Switch = false; 
        if (session_status() === 1){
            $Return_Switch = "Session Disabled";
        }elseif (session_status() === 2){
            $Return_Switch = "Session Enabled, but no sessions exist";
        }elseif (session_status() === 3){
            $Return_Switch = "Session Enabled, and Sessions exist";
        }
        return $Return_Switch;
    }
    public function Set ($Key = false, $Value){
        /*
            Set a value within the $_SESSION global
                this function will return true, if a sucessfull addition has been made
                and return false if a problem is enountered
        */
        if ($this->Session_Started === true){
            $_SESSION[$Key] = $Value;
            return true;
        }
        return false;
    }
    public function Get ($Key){
        /*
            Invoking this function will return the current value of the session key, else return false is an error is encountered
        */
        if (isset($_SESSION[$Key])){
            return $_SESSION[$Key];
        }
        return false;
    }
    public function Display(){
        /*
            If session is started, this function will return a readable and formatted array
        */
        if ($this->Session_Started === true){
            return "<pre>".$_SESSION."</pre>";
        }
    }


} // Close class

Now, for example:

$Class = new Session();

if ($Class->Set("Key","Value")){
  echo "Created Value/key within the Session array, and returned true";
}

I understand that people like to validate a successful/fail boolean based on true and false, so I have included the correct return, but my overall question is just to confirm which I believe is correct.. Which is regarding the returns... Is it a perfectly acceptable thing to do, over having no returns? and within best preference?

\$\endgroup\$
  • 1
    \$\begingroup\$ I’d say the best way to handle that is probably to throw an exception if the session hasn’t been started, unless an unstarted session is going to be part of normal behaviour. \$\endgroup\$ – Ry- Aug 16 '13 at 15:57
2
\$\begingroup\$

As for your question, at least Get should not return false if the value is not set, now the caller will not know whether the actual value is false or whether the value is not set.

Other than that,

  • I would suggest you use PHP_SESSION_DISABLED , PHP_SESSION_NONE and PHP_SESSION_ACTIVE instead of 1, 2 and 3

  • To have Status_Session call session_status is confusing, maybe call it session_status_string ? Also, this should not return false, but maybe "Session status could not be determined"

  • I dont understand this : if (session_status() === 1){ $this->Session_Started = true; if session_status() == 1, then sessions are disabled, why would you set Session_Started to true ?

  • session_start() will return a boolean that indicates whether the session actually started, you should check that boolean instead of assuming success with $this->Session_Started = true;

  • session_start() also can resume a session, I am not a PHP master, but it seems to me you could replace init() with

public function init(){
    /* Start or Resume a session */
    $this->Session_Started = session_start();
    return $this->Session_Started;
}
  • Finally, if you are adamant about checking whether the session is started or not in set, would it not make sense to start the session for the caller, so that your code works auto-magically ?
public function Set ($Key = false, $Value){
  if (!isset($_SESSION)) { 
    init(); 
  }
  $_SESSION[$Key] = $Value;
}

Also, as mini-tech commented, if you expect a session, and it is not there, then you might want to throw an exception instead of returning false.

\$\endgroup\$
  • \$\begingroup\$ I'm not expecting a session. This class is being built to have the ability to migrate into websites which have currently invoked session_start, if a session has already been started. Then a session_start(); will not be called, if not then calling $Class->init() will invoke a session_start(); \$\endgroup\$ – Daryl Gill Aug 16 '13 at 16:35
  • \$\begingroup\$ You are expecting a session any time you use $_SESSION ? \$\endgroup\$ – konijn Aug 16 '13 at 18:26
  • \$\begingroup\$ Alright. Let me explain properly, the construct will detect if session has already started, if it is already started. Then it will force $this->Session_Start = true;, which will in transit stop this class from attempting to start antoher session due to the internal reference pointer being set to true. This is going off topic of the question, i'm here asking if returning a boolean is a good idea or not \$\endgroup\$ – Daryl Gill Aug 16 '13 at 18:53
  • \$\begingroup\$ @DarylGill returning stuff in a __construct is kinda lame... the __construct is called when you are creating a new instance of that class. So the 'returned boolean' is in fact not returned anywhere ;) \$\endgroup\$ – Pinoniq Aug 20 '13 at 13:05
1
\$\begingroup\$

This class isn't really usefull. What if you suddenly decide to not use $_SESSION but a db connection for your session storage? You would then have a lot of duplicate code.

Also the naming of your methods are horrible. Now I would have to read the entire doc/method code to know what really happens. a method nams as getSessionStatus() tells me I'm getting the session status. But Session_Status() doesn't really say a lot, is it for getting? setting? does it toggle the session_status? Oh, it returns a string e.d. representation data. Nah thx, I'll just stick to the session_status() php function.

Then init(). Isn't this simply another name for a __construct()? obviously session's have to be enabled for the class to function. If it isn't enabled on __construct() simply throw an exception and remove that weird init() function and all those if( session_Satrted )

But to answer your original question:returning a boolean in a __construct() is stupid... If you need something that isn't present, throw an Excpetion. If you don't throw an exception then you should work perfectly. Then all methods should have NO SIDE-EFFECTS. If I get a value from the session, I'm only getting a value. I'm not telling you to start a session... So don't do that.

Think DRY SOLID and write your code using a better 'standard': http://www.php-fig.org/psr/1/

\$\endgroup\$
  • \$\begingroup\$ Session_start() is invoked once, there is no secondary invoking within this class.. If you are to get a value, the function checks the internal pointer to see if Session is enabled. \$\endgroup\$ – Daryl Gill Aug 20 '13 at 13:43
  • \$\begingroup\$ @DarylGill and that is wrong. It create overhead. Your get function is also initializing the session... And everytime you call the get function the if is evaluated creating massive overhead. What if I have multiple sessions using session_name()? \$\endgroup\$ – Pinoniq Aug 20 '13 at 14:58
  • \$\begingroup\$ I also added a link to a good standard to use when writing code \$\endgroup\$ – Pinoniq Aug 20 '13 at 15:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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