Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I want to work with the tumblr API, however it looks like I failed at it yet. Do you have any clues for me?

<?php
/**
 * User: dmendek
 * Date: 18.07.12
 * Time: 16:09
 */

class tumblrClass {

    // ----------------------------------------------------------#
    // ----------------------------------------------------------#
    // ----------------------------------------------------------#

    // Blog-Details
    public static $apikey = 'x';
    public static $nameBlog = 'mendavify';

    // Wichtig innerhalb der Klasse
    private static $errorContainer;
    public $jsonArray;

    // ----------------------------------------------------------#
    // ----------------------------------------------------------#
    // ----------------------------------------------------------#

    public static function readInfo() {

        return self::jsonToArray(self::jsonGet('http://api.tumblr.com/v2/blog/'.self::$nameBlog.'.tumblr.com/info?api_key='.self::$apikey));

    }

    public static function readPosts() {

        return self::jsonToArray(self::jsonGet('http://api.tumblr.com/v2/blog/'.self::$nameBlog.'.tumblr.com/posts/text?api_key='.self::$apikey));

    }

    private function jsonGet($url) {

        return json_decode(file_get_contents($url), true);

    }

    private function jsonToArray($json) {

        // short conditionals -> http://tinyurl.com/cl6ru4t
        return (!$json) ? self::throwException(__FUNCTION__, __LINE__) : $json;

    }

    private static function throwException($functionName, $exLine) {

        self::$errorContainer[] = 'Error in function <strong>'.$functionName.'</strong> on line <strong>'.$exLine.'</strong>';

    }

    // Destructor der Klasse, falls Fehler auftraten werden diese ausgegeben
    public function __destruct() {

        if(!empty(self::$errorContainer)) print_r(self::$errorContainer);

    }


}


$tumblr = new tumblrClass();
$tumblr->readInfo();

Thanks in advice.

share|improve this question
4  
This site is for reviewing code that works, which you want improved, so your question doesn't belong here. You should ask your question on StackOverflow, but you will need to include more information, like: How exactly did you fail? What do you expect your code to do? What does your code actually do? – svick Jul 19 '12 at 8:00

closed as off topic by Corbin, svick, mseancole, palacsint, sepp2k Jul 21 '12 at 10:09

Questions on Code Review Stack Exchange are expected to relate to code review request within the scope defined in the FAQ. Consider editing the question or leaving comments for improvement if you believe the question can be reworded to fit within the scope. Read more about closed questions here.

1 Answer

Well, the above comment is mostly right. If this code was at least mostly operational and you wanted to know why something minor was causing an issue, I'm sure none of us would mind, but primarily this site is indeed for reviews only. SO, is for these kinds of questions. The only reason I'm putting an answer here is because I do think you need the review too. I don't know anything about that API to help you out specifically, but I can help you with your syntax.

You want to keep your code short to maintain legibility. So these long return lines, while nice to have on one line, are extremely difficult to read. Break them up into multiple lines. While not as "clean" looking, it is much easier to read.

$url = 'http://api.tumblr.com/v2/blog/';
$blogPath = self::$nameBlog . 'tumblr.com/info';
$key = '?api_key=' . self::$apikey;

$json = self::jsonGet( $url . $blogPath . $key );
return self::jsonToArray( $json );

If you find that you are repeating yourself, as with that URL, you can alleviate some of the repetition by making it a class property. This means you won't have to declare it in each method now, just use it, which also helps with legibility. In other words, in that example above, the $url declaration is now unnecessary, you will only need to access it in the $json variable, either with self:: as you have been doing (which means the below example should be static), or with this->.

public $url = 'http://api.tumblr.com/v2/blog/' . self::$nameBlog . '.tumblr.com/';

This might be more of a stylistic choice, but parenthesis around a condition, unless testing more than one condition, are unnecessary. However, if you are testing more than one condition, you should skip ternary operations all together as they begin to look cluttered.

//(!$json) == ! $json
return (!$json) ? self::throwException(__FUNCTION__, __LINE__) : $json;
//If PHP >= 5.3 you can do this
return $json ?: self::throwException(__FUNCTION__, __LINE__);

Instead of recreating a language construct, the exception, just use the one that already exists. Throw the exception in the class, then catch it during implementation. The only reason to create a custom "exception" is if you want to continue execution in that method afterwards, in which case you wouldn't call it an exception, you'd call it an error or warning.

Also, unless you are inheriting from this class and need to use those values specific to this class, rather than a shared one between parent and child, then use this-> instead of self::. They both work, but self:: explicitly says this information is static, thus the need for the static keyword. This limits inheritance quite a bit. This explanation may be a bit lacking, its from memory, so its the best I could do on short notice. I personally have never needed to use self:: in any of my code, nor static, so I haven't pursued it. The $errorContainer is a good example. This property can only be referenced by this class now. So any child classes will have to refer to the parent's $errorContainer instead of sharing it.

Finally, I hope you will eventually have a better destructor. print_r() and var_dump() are both debugging tools and should not be used in live environments. Instead, I would push the container to a log or an HTML file that would loop over it and produce a visually appealing error screen (as visually appealing as that can be), or both.

share|improve this answer

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