Optimize PHP Class

This post is a bit rambly, but here's a few things that jumped out at me:

Technicalities

private $expiration = "3600";

Why is 3600 a string?

public function __construct( $cache, $api, $callback, $params )

When you're expecting a certain type, use typehints:

public function __construct(Cache $cache, TwitterOAuth $api, callable $callback, $params )

private function get_json( $cache )
{
    if ( $this->validate_cache( $cache ) ) {
        $json = $this->cache->retrieve( $cache );
    }
    return $json;
}

That method is flawed. It's possible for it to execute and $json not be defined.

I think what you're looking for is:

private function get_json( $cache )
{
    if ($this->validate_cache( $cache ) ) {
        return $this->cache->retrieve( $cache );
    } else {
        return null;
    }
}

private function has_callback()
{
    if ( isset( $this->callback ) ) {
        $callback = true;
    }
    return $callback;
}

Same problem as your get_json.

And, it can be simplified to:

private function has_callback()
{
    return (isset($this->callback));
}

Although, really, since you know that $this->callback always exists, you could use return $this->callback !== null; if you wanted.

if ( isset( $this->callback ) ) {

That probably doesn't do what you think it does. When $this->callback = '', that's going to return true. That means that your jsonp stuff would return the non-sensical (...);

$config = (object) array(
    'callback' => $_GET['jsonp_callback'],
    'params'   => $_GET,
);

There's no reason to use an object here. Just use an array.

Also, I'm assuming that this is just sample code, but in real code, you shouldn't assume that indexes exist. If jsonp_callback isn't provided, this will issue a notice.

Design

private function twitter_api_call( $status, $params )
{
    return $this->twitter->get( $status, $params );
}

I suppose as long as this stays private there's nothing particularly wrong with it. Seems a bit overkill though other than to save a few keystrokes.

private function set_header( $header )
{
    switch ( $header ) {
        case "json":
            $header = header( "content-type: application/json" );
            break;
    }
    return $header;
}

This method has quite a few problems:

• The name is wrong:
  • It doesn't set a header, it sends a header
  • $header isn't a header
• header() doesn't return anything
• Your switch, and by extension the $header parameter are useless. The method could just be called sendJsonHeader.
• Your class probably shouldn't be responsible for sending headers. That should happen one or more levels up.
• What if you wanted to embed the JSON content in JavaScript in an HTML page? Your class has then sent a header you don't want. (This is actually a problem in data())

/**
 * Public interface
 *
 * This Method return cached data to the client.
 * 
 * @access public
 * @return object
 */
public function get_data()
{
    return $this->data();
}

There's no need to comment that it's public. Any IDE or program capable of parsing docs can parse visibilities.

Also, if the code just proxies through to data(), perhaps get_data() should house data()'s code, and data() shouldn't exist.

The public API of https://github.com/cosenary/Simple-PHP-Cache isn't too bad (it's not great either though), but the interal code has quite a few "wtf?" snippets. I might consider using a different cache. (Also, this cache isn't very flexible. What if at some point your site became super popular and you want to move your cache from disk to memcached? There are a lot of cache classes that could handle that with 1 line of code changed.)

The JSON/JSONP shouldn't be per-instance of the class, but rather per each time you call the data() method. What if you want to get the data as JSON once and JSONP once? Seems pointless to create a new object or have to set the callback.

That brings me to my next point. This is a bit of an odd one since it's basically the purpose of your class, but your class probably shouldn't be returning JSON.

Would you ever expect a method to return HTML? What about XML? So why does your method return JSON? Seems a bit odd and inflexible.

At the same time though, your class could be seen as a renderer, I suppose. Renders are responsible for actually return HTML/XML/etc. The problem with that though is that rendering should be the only thing done. Your class is mixing the responsibilities of getting data and rendering said data.

I would probably just return an array, or perhaps an object that models the data. That way, the consuming code could decide if it wants JSON or not.

Either always pass a parameter to methods when necessary, or always use it as a property of the class. Mixing and matching those two approaches is a good sign that the property probably shouldn't actually be a property but rather should be passed to methods every time they're called.

$params is a good example of this. The way it's being passed around, it feels like it should probably be a parameter to the data() method.

Upon further examination, this is almost certainly true. What if you wanted to get data for two different usernames? Right now you'd have to create two different objects. That doesn't make sense though, unless the class purposely represents the data for only one username.

CTweet and data() are badly named. It might just be because I've been on twitter a grand total of about twice, but I have no idea what CTweet is (cached tweet?) nor what data() returns (the "cached data" in the documentation is very, very vague -- when you write your doc blocks, assume you've never seen the class before and are trying to figure out how to use it).

A few other things about the overall design make me a bit uncomfortable, though I can't manage to pin point exactly what it is.

I think the caching being intermingled with the API-interaction is part of it, though I unfortunately do not have much of a suggestion as to handle it better.

I would probably keep the caching out of the class and just have your consuming code handle the caching. It seems like embedding the caching in a class inherently gives the class two responsiblities: caching, and whatever its original responsibility was.

The problem with having the consuming code cache though is that you will get code duplication if you are consuming the code in two places. I suppose that's not really a problem though because that code should be factored out into a common method or some notion of an action if it's being duplicated.

Overall, I'm not sure what the best way to handle caching is; I just know that the current way makes me a bit uneasy. (Think about it this way: how much of a pain would it be to toggle caching on and off? It should be simple in concept. Just kill the caching. In practice with this code though, it would get a bit painful.)

I guess what might actually be making me uncomfortable is the entire class. What if you want to cache another snippet from the twitter api? Are you going to make an entirely new class? What if you want to use the data not as json? What if you wanted to use a different type of caching? What if you didn't want to cache? The class seems to have completely garbled together using the twitter API and caching. It seems like the class is trying to do two completely unrelated things (using the twitter API/caching data), and I'm afraid that mixture won't be manageable in the long run.

Edit: Just read my post back, and wow, it's quite wordy/badly written. Am going to revise it later, but don't have time for now. Hopefully there's some clarity through the ramble :).