3
\$\begingroup\$

I have written a script that checks if data is known in memcached, and if not it queries it from the mysql db and stores it.

I would appreciate any inputs if i have done this the "correct" way, since i'm trying to optimize the code as much as possible.

_query() & _rows() are simple mysqli functions

Save in memcache

// Store memcache data
function setCache($key,$value,$storage=18) {
    $memcache_obj = new Memcache;
    $memcache_obj = memcache_connect(memcache_host, 11211);

    if(is_array($key)) {
        $i = 0;
        foreach($key as $id => $storeName) {
            $memcache_obj->set($storeName, $value[$id], 0, $storage);   
        }
    }else{
        $memcache_obj->set($key, $value, 0, $storage);
    }
}

Get from memcache

// Get memcache data
function getCache($key) {
    $memcache_obj = new Memcache;
    $memcache_obj = memcache_connect(memcache_host, 11211);
    $res = $memcache_obj->get($key);
    if(empty($res)) {
        $res = false;   // Expired
    }
    return $res;
}

Get data from memcache (or store in memcache if not present)

// Get data from either memcache or mysql
function _getData($userid,$table,$fields,$server) {

    $toSelect = explode(",",$fields);
    $toPush = array();
    foreach($toSelect AS &$value) {
        // Check if data is available from cache
        $key = $userid."_".$table."_".$value;
        $res[$value] = getCache($key);
        if(empty($res[$value])) {
            // Not cached, so must be pushed
            $toPush[] = $value;
        }
    }

    if($toPush) {
        // Some or all data missing from cache, so we fetch and cache it
        $fieldsToSelect = implode(",",$toPush);
        $q = _query("SELECT ".$fieldsToSelect." FROM ".$table." WHERE id = '".$userid."'",$server);
        $row = _rows($q);
        $key = array();
        $cValue = array();
        foreach($toPush AS &$value) {
            $key[] = $userid."_".$table."_".$value;
            $cValue[] = $row[$value];
            $res[$value] = $row[$value];
        }
        setCache($key,$cValue);
    }
    return $res;

}

The idea is to use the function as follows:

_getData($userid,"name_table","first_name,last_name",$dbConnection);

The above will return the data requested from memcahced first and if not present, then query it from mysql and store it in memcached.

The above works well, but how much does it hurt the eye?

\$\endgroup\$
2
\$\begingroup\$

Four things come to mind:

  1. You're mixing object oriented programing with the more 'traditional' way PHP was written. You don't need to use both. If you will use the older memcache_connect, memcache_get and memcache_get functions, you don't need to instantiate an object with new Memcache. Checkout the bottom of the connect page for the two side by side. Instantiating gives you more flexibility though (see point 3 below). You tend to call memcache_connect even though the rest of your code is object-oriented; be consistent and use $memcache_obj->connect(...)

  2. memcache_get returns FALSE when the object is not in the cache. So there is no need in your getCache function to do if(empty($res)){$res=false} because $res will already be false.

  3. Consider closing the connection to the cache when you're done with it. Otherwise you may have a large number of active connections as you create a new instance and connect every time getCache and setCache are called. I would call Memcache::close before returning from either function. Alternatively, if these functions are called often within one request-response cycle, you could have a Cache class that you instantiate; the Memcache instance would be a property of the class. Then in the class constructor, you can call new Memcache and Memcache::connect, and use the same connection for all operations. Then in the class destructor (the __destruct function, you can close the connection.

  4. Your _getData indiscriminately uses caching. No matter what data is sought, it gets cached. You may have to be a bit more surgical because if you have fast changing DB data, _getData will soon start returning obsolete information (for instance, one would not seriously consider serving current stock prices or currency conversion rates from a cache). You may want to apply caching only to a subset of your DB, depending on the nature of your data

\$\endgroup\$
4
  • \$\begingroup\$ Great info, thank you very much. In terms of performance, is there any disadvantage to combining OOP with procedural, as you mention in "1" ? \$\endgroup\$
    – JPJens
    Jun 12 '16 at 18:41
  • \$\begingroup\$ I don't know and don't want to speculate. I encouraged you to be consistent because complexity breeds bugs. You want your code to be simple to follow in order to reduce errors and improve maintainability. Out of curiosity, how did you come up with 18 seconds as the default storage lifespan in setCache? \$\endgroup\$ Jun 13 '16 at 8:39
  • \$\begingroup\$ Great info. Thanks again. The 18 seconds are for testing purpose. Enough to see it being stored and Enough to get fresh data while developing. (could just as well have been 20) \$\endgroup\$
    – JPJens
    Jun 13 '16 at 8:53
  • \$\begingroup\$ Ok. Give that a lot of thought before you deploy, especially when it comes to storing DB data in the cache indiscriminately (your _getData always stores/fetches to/from the cache regardless of what info is sought). I ran into trouble overusing the cache to improve performance and my operations would return stale data (that no longer matched what was in the DB). So I'm always trying to find that equilibrium; I don't treat all DB data the same vis a vis caching. \$\endgroup\$ Jun 13 '16 at 8:59

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.