HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Store all entries in a single array from Wufoo api

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
allarraystoresinglefromapiwufooentries

Problem

What I'm doing

Using the v3 of the Wufoo API to retrieve all entries and store in a single array ($all_results). This is necessary because of the limit 100 entries limit that Wufoo has when retrieving results from their api. Also store the results to file we don't get slow responses from the api.

My question

I'm wondering how I could have done this better. This code is working for me, but I want to get better.

 $counted) ? $limit - (($offset + $limit) - $counted) : $limit;

            // Iterate through the Entries array to push each entry into a new array
            for ($result = 0; $result ";
print_r(wufoo_entries());
echo "";

?>

Solution

Just some quick note:

1, I would separate the configuration settings to a distinct php file (and use a WufooConfig class/struct) and include it at the beginning of the code. It makes the configuration easier to change. If you use a WufooConfig class/struct and pass it to the wufoo_entries() function you will be able to use the same function to fetch data from more than one accounts. Furthermore, it would encapsulate configuration, so you just have to pass around one parameter instead of $account, $form, $api_key, etc.

class WufooConfig {
    private $account;
    private $form;
    private $api_key;
    ...

    public function __construct($account, ...) {
        $this->account = $account;
        ...
    }
}


2, Extract the then and else branches of the if in the wufoo_entries() function:

if ($refresh) {
    $results = fetchContent(...);
    saveToCache($results, ...); // write file here
    return $results
} else {
    return getCachedContent();
}


It makes your code easier to read, you'll see the big picture immediately, it won't be distracted with low-level API calls.

For the same reason I'd create some new functions, for example:

functio entryCount(...)
    $count = wufoo_api($api_key, $account, $form, $offset, $limit, $api_uri);
    $counted = $count['EntryCount'];
    return $counted;
}


The name of the function says what you want to get and the body shows how you do it.

3, Change

for ($offset = 0; $offset <= $counted - 1; ...


to

for ($offset = 0; $offset < $counted;


It's more common.

4, In the inner for loop the $result loop variable is a little bit confusing, it's easy to mix up with $results. $resultIndex would be better.

for ($resultIndex = 0; $resultIndex < $limit; $resultIndex++) {  
    array_push($all_results, $results['Entries'][$resultIndex]);
}


Finally, I agree with @Yannis Rizos, the whole logic would be better as a class.

Code Snippets

class WufooConfig {
    private $account;
    private $form;
    private $api_key;
    ...

    public function __construct($account, ...) {
        $this->account = $account;
        ...
    }
}
if ($refresh) {
    $results = fetchContent(...);
    saveToCache($results, ...); // write file here
    return $results
} else {
    return getCachedContent();
}
functio entryCount(...)
    $count = wufoo_api($api_key, $account, $form, $offset, $limit, $api_uri);
    $counted = $count['EntryCount'];
    return $counted;
}
for ($offset = 0; $offset <= $counted - 1; ...
for ($offset = 0; $offset < $counted;

Context

StackExchange Code Review Q#6049, answer score: 2

Revisions (0)

No revisions yet.