2
\$\begingroup\$

Below are two functions.

The first is to set a cookie. This function is called on any product page on my website.

function setcookie() {

    $entry_id = $this->EE->TMPL->fetch_param('entry_id');

    if (isset($_COOKIE['recently_viewed'])) {

        $currentSession = unserialize($_COOKIE['recently_viewed']);

        if (!in_array($entry_id, $currentSession)) {

            if (count($currentSession) > 5) {
                unset($currentSession[0]);
            }

            $currentSession[] = $entry_id;

        } else {}

        $currentSession = serialize($currentSession);
        setcookie('recently_viewed', $currentSession, pow(2,31)-1, '/', '');

    } else {

        $recently_viewed[] = $entry_id;
        $currentSession = serialize($recently_viewed);
        setcookie('recently_viewed', $currentSession, pow(2,31)-1, '/', '');

    }

}

The second function retrieves the IDs in the cookie array and outputs them.

function recently_viewed() {

$tagdata = $this->EE->TMPL->tagdata;

if ( array_key_exists('recently_viewed', $_COOKIE) ) {
    $recent = unserialize($_COOKIE['recently_viewed']);
} else {        
    $recent = NULL;
}   

if ($recent) {
    $entry_ids = implode('|', $recent);
} else {
    $entry_ids = NULL;
}

$data['entry_ids'] = $entry_ids;

$variables = array();
$variables[] = $data;

return $this->EE->TMPL->parse_variables($tagdata, $variables);

}

The code is built for the content management system Expression Engine. If you don't know 'codeigniter' code, just ignore those bits and take a look at the straight PHP if possible.

Can anyone tell me if there is any way to speed the functions up?

The first function, setcookie, is basically checking to see if a cookie exists. If it does, it checks if the current entry id is in the array already. If it's not, then it checks if there are more than 5 items in the array. If there is, it deletes the first one and adds the new entry id. If no cookie exists, it creates a new cookie and adds the entry id.

The second function (recently viewed) is checks the cookie, converts it to a string with pipelines between entry ids, then outputs this string.

\$\endgroup\$
1
  • \$\begingroup\$ Why do you use cookies instead of sessions to store this data? \$\endgroup\$ – bumperbox May 8 '14 at 21:09
2
\$\begingroup\$

First of all, let me tell you that I have zero experience with Codeigniter, but I suppose it is an MVC / OOP based framework, and I have plenty of experience with those. I am not trying to declare myself an expert or anything, just trying to be helpful and constructive. So here we go...

A few general words of advice:
- try to avoid nesting control structures. Work with early returns in stead. You hardly ever need to nest an if inside an if, or even use an else. If you realy do, you are probably not following the principles of single responsibility
- don't waste resources by storing stuff in variables and only using them once immediately after you stored them. Use the value you wanted to store directly.
- Put comments inside your code. A lot of them! It may take a little bit longer now, but it will safe you time when you ever have to debug or alter your code!

With those hints in mind, I would write your first function as follows:

function setcookie() {  
    // prepare the entry id
    $entry_id = $this->EE->TMPL->fetch_param('entry_id');

    // if cookie has not been set before
    if (! isset($_COOKIE['recently_viewed'])) {
        // write the current entry id to a cookie
        setcookie('recently_viewed', serialize(array($entry_id)), pow(2,31)-1, '/', '');
        // and we are done (return self for chaining)
        return $this;
    }

    // convert the cookie to an array
    $currentSession = unserialize($_COOKIE['recently_viewed']);

    // if the current entry id is already in the cookie
    if (in_array($entry_id, $currentSession)) {
        // we are done (return self for chaining)
        return $this;
    }

    // if there are already 5 entries
    if (count($currentSession) > 5) {
        // shift of the first entry 
        // don't use an index here, cause there is no garantee the first element actualy has index 0
        array_shift($currentSession);
    }

    // push the current id onto the list
    $currentSession[] = $entry_id;
    // write it to a cookie
    setcookie('recently_viewed',serialize($currentSession), pow(2,31)-1, '/', '');
}

And the second function:

function recently_viewed() {
    // prepare the tagdata
    $tagdata = $this->EE->TMPL->tagdata;

    // if no recently_viewed cookie is present
    // (isset is supposed to be a little better in performance then array_key_exists)
    if (! isset($_COOKIE['recently_viewed']) ) {
        // do something and return the result
        return $this->EE->TMPL->parse_variables(
            $tagdata, 
            array(
                array('entry_ids' => null)
            )
        );
    }   

    // convert the recently viewed ids to a piped string
    // do something with it and return the result
    return $this->EE->TMPL->parse_variables(
        $tagdata, 
        array(
            array(
                'entry_ids' => implode('|', unserialize($_COOKIE['recently_viewed']))
            )
        )
    );
}

You could even put that function into a single 'line' of code, which should be a bit more performant, but a lot less readable. And imo you should always take readable code over performant code when the difference is small. Just as an example though:

function recently_viewed() {
    return $this->EE->TMPL->parse_variables(
        $this->EE->TMPL->tagdata,
        array(
            array(
                'entry_ids' => 
                    isset($_COOKIE['recently_viewed']) 
                    ? implode('|', unserialize($_COOKIE['recently_viewed'])) 
                    : null
            )
        )
    );
}
\$\endgroup\$

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.