Is there a better way to loop through records and call an api?

Question

I have a CakePHP function here that gets info for a twitter screen_name from the twitter api and writes it to a mysql db. I'm just looking for feedback about other ways to accomplish this that might be more efficient with fewer loops maybe. The challenge is, there are many screen_names that need to be processed for a $reportid before the "report" is considered done. Any feedback is very much appreciated. thanks.

    function admin_wtfTwitter($reportid, $jobid) {

    $this->autoRender = false;

    //use if the api lags
    //set_time_limit(18000);

    //notify the log
    $this->admin_saveLog($reportid, $jobid, 'parsecrawlresults', 'Twitter wce started.');

    //get the twitter channels from reportdetails
    $channels = $this->Reportdetail->getChannel($reportid, 4);  //channel 4 = twitter
    foreach ($channels as $c) {

        //open reportdetail channel for writting
        $this->Reportdetail->create();
        $this->Reportdetail->set(array('id' => $c['Reportdetail']['id']));

        /*
        *   small_tag is null --> continue to next reportdetail record
        *   small_tag is bad
        *   small_tag is good but no data returned
        */
        if ($c['Reportdetail']['small_tag'] == null) { $this->Reportdetail->set(array('done' => -1, 'note' => 'small_tag null')); $this->Reportdetail->save(); continue; }

        $uri = 'http://api.twitter.com/1/users/show.json?screen_name=' . $c['Reportdetail']['small_tag'];
        $data = @file_get_contents($uri);
        if($data === false) { unset($data); $this->Reportdetail->set(array('done' => -1, 'note' => 'small_tag bad')); $this->Reportdetail->save(); continue; }
        $content = json_decode($data, true);

        //set fans, title username
        $this->Reportdetail->set(array(
            'fans' => $content['followers_count'],
            'title' => $content['description'],
            'username' => strtolower($content['screen_name'])
        ));

        //proceed with second api call****************************
        unset($uri);
        unset($data);
        unset($content);
        $i = 0;
        $e = 0;
        $postmonths = array();
        //LIMIT SET TO 1000 - CAN BE 3200 - https://dev.twitter.com/docs/api/1.1/get/statuses/user_timeline
        $uri = 'https://api.twitter.com/1/statuses/user_timeline.json?trim_user=true&exclude_replied=true&include_entities=true&include_rts=true&contributor_details=false&count=1000&screen_name=' . $c['Reportdetail']['small_tag'];
        $data = @file_get_contents($uri);
        if($data === false) { unset($data); $this->Reportdetail->set(array('done' => -1, 'note' => 'small_tag bad')); $this->Reportdetail->save(); continue; }
        $content = json_decode($data, true);            

        //set engagement, posts
        foreach ($content as $t) {
            if (!empty($t['id_str'])) { 
                $i++;
                $e = $e + $t['retweet_count'];
                array_push($postmonths, substr($t['created_at'], 4, 3)); //API resutls are: "created_at":"Fri Nov 30 00:38:34 +0000 2012
            }
        }
        $this->Reportdetail->set(array(
            'posts' => $i,
            'engagements' => $e
        ));

        //get and set frequency
        $freq = $this->admin_calcFrequency($i, count(array_unique($postmonths))); 
        $this->Reportdetail->set(array('frequency' => $freq));

        //********************************************************

        //we're done with this one
        $this->Reportdetail->set(array('done' => 1));

        //commit the content to reportdetail record
        $this->Reportdetail->save();

    }

    //notify the log
    $this->admin_saveLog($reportid, $jobid, 'parsecrawlresults', 'Twitter wce completed.');
}

Answer 1

There are two fundamental principles I would like to point out: "Don't Repeat Yourself" (DRY) and Single Responsibility. The principles should be self explanatory from their names. The very first thing I notice is that you have a "god" function. In other words, a function that is trying to do everything. This violates both of these principles. DRY because the code cannot be easily reused; Single Responsibility because functions should do just one thing. To avoid this you should separate this one function into multiple smaller ones. The best way to do this is to try and determine each individual task you are trying to accomplish and separate accordingly. I'll leave that bit up to you.

A small thing real quick before continuing. This is actually a method, not a function, and therefore the access modifier should be explicitly defined as public, private, or protected to avoid accidents. By default it resolves to the public scope, but you should not depend upon that.

public function admin_wtfTwitter() {

Internal comments are very detrimental to legibility and code size. You should avoid them except when debugging. If your code is self-documenting then most comments will become unnecessary and you will only need doccomments for the rest. Comments like "notify the log" or "get the twitter channels" should be inherently obvious from the context. If you do nothing else, removing these comments alone will dramatically increase overall legibility.

You should also avoid (un)commenting out a feature to simulate different environments. If you have a feature that you will sometimes need to activate, you should create a switch in your function's arguments to handle it. For instance:

function admin_wtfTwitter( $reportid, $jobid, $lag = FALSE ) {
    if( $lag ) {
        set_time_limit( 180000 );
    }

Though I should caution that you shouldn't override your time limit like that. First of all, it makes your server hang while it waits for the task to finish. Second, it could mask a much larger problem, such as an infinite loop. If your program is doing so much that it can't finish, then that should be a sign that there is something wrong and you should fix it, not mask it. There are a number of different methods you can use: manually limit the initial amount of work, dynamically limit it by breaking it up into chunks, and caching results to name a few. There are probably more, but those are the ones I'm familiar with. The method you chose depends upon your situation.

You should avoid single letter variables except when used in generic throw away values or incrementals. Using $c instead of $channel is rather abstract and can later lead to confusion, especially in larger chunks of code. The same can be said for non-common acronyms, in other words acronyms that are not HTML, PHP, etc...

Whitespace is your friend and PHP never minds a little over-zealousness in its use. Never cram multiple statements onto one line, even if it is legible, but especially avoid it when its not. Having more than one level of indentation wont hurt. That being said you should be aware of the arrow anti-pattern and ensure you aren't violating it.

if( $channel[ 'Reportdetail' ] [ 'small_tag' ] == NULL ) {
    $this->Reportdetail->set( array(
        'done' => -1,
        'note' => 'small_tag null'
    ) );
    $this->Reportdetail->save();
    continue;
}

Lets return to the first principle I mentioned: DRY. There are a few subtle aspects that are sometimes overlooked. Usually this principle is applied by breaking code up into functions or loops and most think that is enough. But it can also be used for something as simple as saving a reused array pointer to a variable. So, instead of continuously typing out $c['Reportdetail']['small_tag'], save that value to a variable, say $small_tag and use that instead. Later, should you decide to change the source or path, the initial value of the variable is the only line you will have to worry about.

Continuing on this line of thought, if you are setting multiple values to a resource, then the best way is to do it all at once. Not only does this follow DRY, it also saves processing time. Currently you are using $this->Reportdetail->set() method repeatedly to set multiple attributes. Always avoid accessing or manipulating resources multiple times when at all possible.

$attributes = array(
    'id' => $channel[ 'Reportdetail' ] [ 'id' ]
);

if( $small_tag == NULL ) {
    $attributes[ 'done' ] = -1;
    $attributes[ 'note' ] = 'small_tag null';

    $this->Reportdetail->set( $attributes );
    $this->Reportdetail->save();
    continue;
}

You can even go one step farther and create a "default" state. For example, if the $small_tag is NULL or bad the "done" attribute is set to "-1" and there is a "note" attribute. So if we set up our $attributes array with the null values as a default, we can manipulate or unset attributes as necessary. This sometimes even allows us to remove an if statement if done right.

$attributes = array(
    'id'   => $channel[ 'Reportdetail' ] [ 'id' ],
    'done' => -1,
    'note' => 'small_tag null'
);

$uri = "http://api.twitter.com/1/users/show.json?screen_name=$small_tag";
$data = file_get_contents( $uri );
if( $data === FALSE ) {
    if( $small_tag ) {
        $attributes[ 'note' ] = 'small_tag bad';
    }

    $this->Reportdetail->set( $attributes );
    $this->Reportdetail->save();
    continue;
}

$attributes[ 'done' ] = 1;
unset( $attributes[ 'note' ] );

There are a few things I want to point out about what I demonstrated above. The most noticeable is the new if statement structure. If we assume that file_get_contents() will return FALSE if $small_tag is NULL then we can reuse the set/save/continue for both "bad" statements, all we have to do is apply the proper "note" depending upon the value of $small_tag. There shouldn't be any noticeable difference in processing time as it should immediately fail, but you may want to check that.

Something a little less noticeable is the removal of the error suppressor @. First of all, you should never use the error suppressor. You should always explicitly check for errors and handle them. However, it isn't even necessary here. file_get_contents() only throws errors if using the maxlength or offset arguments, neither of which you are. The only "bad" return you have to worry about is a FALSE one.

Another not so obvious change is the removal of the unset( $data ); line. Micromanaging memory like this is unnecessary. The benefit is so negligible as to be nearly useless and the added clutter detracts from even that small appeal. The only reason to unset a value is if you may end up using it later and don't want it to have a value first. If you are in a loop and the next iteration of that loop automatically overwrites the previous values, then unsetting is unnecessary. This includes unsetting $uri and $content.

The final thing I wanted to point out is that you are recreating a boolean by using numerical TRUE/FALSE values with your "done" attribute. The difference between using an actual boolean and a pseudo-boolean is small but the abstractness can cause confusion. You should just use a real boolean unless absolutely necessary.

The rest is more of the same. There's only one last thing I noticed, and it goes along with what I said about the arrow anti-pattern earlier: Watch for unnecessary indentation. If you can reverse an if statement to reduce the amount of indented code then do so. For example:

foreach( $content as $t ) {
    if( empty( $t[ 'id_str' ] ) ) { 
        continue;
    }

    //etc...
}

Hope this helps