Problem
I’m looking for a better way to provide configuration options and improvements in general. I’m using the constructor injection pattern to get access to the Cache and Twitter class. $callback and $params are both part of an $_GET array.
I’m not happy with $config at all. Maybe it’s better to add the whole $_GET array? But i’m not sure how to handle the JSONP Callback.
By the way the callback is added by jQuery and necessary to return a valid JSONP.
Instantiation:
// setup config object
$config = (object) array(
'callback' => $_GET['jsonp_callback'],
'params' => $_GET,
);
// new instance of Cache
$cache = new Cache();
// new instance of TwitterAuth
$twitter = new TwitterAuth();
// new instance of tweet wrapper
$ctweet = new CTweet($cache, $twitter, $config->callback, $config->params);
Array Data
Array
(
[jsonp_callback] => jQuery1820268558845622465_1354013902493
[include_entities] => true
[include_rts] => true
[screen_name] => my_screen_name
[count] => 3
)
The array is built by jQuery. Excluding the [jsonp_callback] all array entries are to retrieve API Data by the TwitterAuth class.
Complete Class
/**
* CTweet
*
* This class gets json data from the twitter api,
* stores them into a flat file and returns the
* data as jsonp.
*
* @version 1.0
*/
class CTweet
{
/** @type string Cache expiration time */
private $expiration = "3600";
/** @type string Twitter API Status */
private $api_status = "statuses/user_timeline";
/** @type object Instance of Cache */
private $cache;
/** @type object Instance of TwitterOAuth */
private $twitter;
/** @type array TwitterOAuth API Params */
private $params;
/** @type object JSONP Callback */
private $callback;
/**
* Constructor
*
* @access public
* @param object $cache
* @param object $twitter
* @param string $callback
* @param array $params
*/
public function __construct( $cache, $api, $callback, $params )
{
if( is_array( $params ) ) {
$this->params = $params;
}
$this->cache = $cache;
$this->twitter = $api;
$this->callback = $callback;
$this->cachename = $this->cache->getCache();
}
/**
* Cache validation
*
* This Method deletes expired entries and regenerates cached data.
*
* @access private
* @param $cache string
* @return boolean
*/
private function validate_cache( $cache )
{
$this->cache->eraseExpired();
if ( ! $this->cache->isCached( $cache ) ) {
$this->refresh_cache();
}
return true;
}
/**
* Cache refreshing
*
* This Method generates the cache file and sets the cachname, data
* and the expiration time.
*
* @access private
* @see Cache::store() https://github.com/cosenary/Simple-PHP-Cache
*/
private function refresh_cache()
{
return $this->cache->store( $this->cachename, $this->twitter_api_call( $this->api_status, $this->params ), $this->expiration );
}
/**
* Get Cache
*
* This Method returns the cached data if cache validation was successfull
*
* @access private
* @param string
* @see Cache::retrieve() https://github.com/cosenary/Simple-PHP-Cache
* @return object
*/
private function get_json( $cache )
{
if ( $this->validate_cache( $cache ) ) {
$json = $this->cache->retrieve( $cache );
}
return $json;
}
/**
* Retrieve Data
*
* This Method retrieves and returned data from Twitter API.
*
* @access private
* @param $status string api status
* @param $params array return parameters
* @see TwitterOAuth::get() https://github.com/abraham/twitteroauth/blob/master/DOCUMENTATION
* @return object
*/
private function twitter_api_call( $status, $params )
{
return $this->twitter->get( $status, $params );
}
/**
* Has callback
*
* This Method returns true only if a JSONP Callback is available
*
* @access private
* @return boolean
*/
private function has_callback()
{
if ( isset( $this->callback ) ) {
$callback = true;
}
return $callback;
}
/**
* Output mimetype
*
* This Method returns a specified file header
*
* @access private
* @param $header string
*/
private function set_header( $header )
{
switch ( $header ) {
case "json":
$header = header( "content-type: application/json" );
break;
}
return $header;
}
/**
* JSONP callback
*
* This Method creates the a jsonp callback
*
* @access private
* @param $cache string
* @return jsonp
*/
private function get_jsonp( $cache )
{
return $this->callback . '(' . $this->get_json( $cache ) . ');';
}
/**
* Data
*
* This Method sets output mime type and returns jsonp if a callback is
* available, otherwise the return value is plain json.
*
* @access private
* @return object
*/
private function data( $cache )
{
if ( $this->has_callback() ) {
$data = $this->get_jsonp( $this->cachename );
} else {
$data = $this->get_json( $this->cachename );
}
$this->set_header( "json" );
return $data;
}
/**
* Public interface
*
* This Method return cached data to the client.
*
* @access public
* @return object
*/
public function get_data()
{
return $this->data();
}
}
Solution
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
$headerisn’t a header
header()doesn’t return anything- Your switch, and by extension the
$headerparameter are useless. The method could just be calledsendJsonHeader. - 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 :).