PHP - is my code efficient?

Question

I am basically getting data from various APIs and using PHP to put them together - like a web mashup. I am currently using 4 foreeach statements to insert the gathered data into their individual arrays. I believe that the current code is inefficient because it takes probably around 3 seconds to load the page which is displaying the PHP data. In the past I had just one big foreach loop to go through all the data at once and also print them. But that too felt inefficient to me.

So how can I make my code more efficient in term of it processing faster? I have seen a few mashup websites such as Soundeer which load around a second. Is that becuase of their code efficiency?

The code which I am using is:

$echonest_uri = simplexml_load_file("http://developer.echonest.com/api/v4/artist/search?api_key=$APIkey&style=rap&results=10&start=$element_num&bucket=id:deezer&bucket=images&sort=familiarity-desc&format=xml");

//Swap the comments for when in UWE or not
//$echonest_xml =  new SimpleXMLElement($echonest_uri);
$echonest_xml = $echonest_uri;

$artist_name = array();
$artist_image = array();
$echonest_id = array();
$full_deezer_id = array();
$deezer_id = array();
$num_of_albums = array();

//Loop through each entries in the id_arr and make each image of the artist a link to the album page passing all the relevant information. 
foreach($echonest_xml->artists->artist as $artist){
    $artist_name[] = $artist->name;
    $artist_image[] = $artist->images->image[0]->url;
    $echonest_id[] = $artist->id;
    $full_deezer_id[] = $artist->foreign_ids->foreign_id->foreign_id;
}

foreach($full_deezer_id as $key => $value){
    preg_match('#deezer:artist:([A-Z,a-z,0-9]+)#', $value, $id);
    $deezer_id[] = (string)$id[1];
}

foreach($deezer_id as $id_index => $id){
    $deezer_xml = simplexml_load_file("http://api.deezer.com/2.0/artist/$id/albums&output=xml");
    $num_of_albums[] = $deezer_xml->total;
}

//The variable which will contain the HTML code to display the artists.
$output = null;


foreach($deezer_id as $key => $value){

    $fav_count_query = "SELECT COUNT(user_index) FROM fav_artist WHERE artist_deezer_id = '$value'";
    $fav_count_resource = $mysqli->query($fav_count_query);
    $fav_count = $fav_count_resource->fetch_assoc(); 

    $output .=  <<<HERE
                <div class="artist-container">
                    <a href="albums.php?echonest_id={$echonest_id[$key]}&deezer_id={$deezer_id[$key]}&artist_name={$artist_name[$key]}&artist_image={$artist_image[$key]}&num_of_albums={$num_of_albums[$key]}" class="artist-image">
                        <img src="{$artist_image[$key]}" alt="{$artist_name[$key]}" title="{$artist_name[$key]}"/>
                    </a>

                    <a href="albums.php?echonest_id={$echonest_id[$key]}&deezer_id={$deezer_id[$key]}&artist_name={$artist_name[$key]}&artist_image={$artist_image[$key]}&num_of_albums={$num_of_albums[$key]}" class="artist-name">
                        {$artist_name[$key]}
                    </a>
                    <a href="albums.php?echonest_id={$echonest_id[$key]}&deezer_id={$deezer_id[$key]}&artist_name={$artist_name[$key]}&artist_image={$artist_image[$key]}" class="album-number">Albums: 
                        {$num_of_albums[$key]}
                    </a>
                </div>
HERE;

}

Multiple consecutive loops is only a problem if each of the arrays is really large, like say ~ 10k elements or more. Otherwise, the network overhead will be much higher than the actual processing time for your script. — lethal-guitar
– lethal-guitar, Commented Jan 19, 2013 at 20:24
I want to ask about this query: SELECT COUNT(user_index) FROM fav_artist WHERE artist_deezer_id = '$value'. Do you need count of all records? Is user_index NOT NULL? — Viacheslav Kondratiuk
– Viacheslav Kondratiuk, Commented Jan 19, 2013 at 20:37
user index is a column name in my DB. I want to count all the user ids (user_index) from fav_artist. — Human
– Human, Commented Jan 19, 2013 at 20:42
Have you considered perhaps that the MySQL query in the loop could be a problem? :) — Ja͢ck
– Ja͢ck, Commented Jan 19, 2013 at 21:24

h2ooooooo · Accepted Answer · 2013-01-19 20:25:56Z

Your code most likely isn't slow because of multiple foreach loops (honestly, you wouldn't feel the difference in a realistic scenario). What's hurting you here is downloading something from an external site.

My solution would be to download this automatically every 5th minute (either via a cronjob, or just when the user accesses the page), and if it's less than 5 minutes ago, then show a cached version placed on the server.

That said, this seems to be a search, so the results most likely wouldn't change a lot. Maybe have a 1 day cache instead? (It would still take 3 seconds per new search query, but if you expect many users to make the same query, then this will make your site seem a lot quicker).

An easy way of testing where in your code it is that something goes wrong timewise is using the microtime function. What I usually do is something like this:

$beforeCall = microtime(true);
$echonest_uri = simplexml_load_file("http://developer.echonest.com/api/v4/artist/search?api_key=$APIkey&style=rap&results=10&start=$element_num&bucket=id:deezer&bucket=images&sort=familiarity-desc&format=xml");
echo "The call took " . number_format(microtime(true) - $beforeCall, 4) . " seconds";

If you're using XML (which is seems like) you can use SimplePie with cache enabled. A good article for this is SimplePie's own article on their cache system where you can adjust the cache duration with set_cache_duration.

Community · Accepted Answer · 2020-06-20 09:12:55Z

Caching

Of most relevance, the thing you are missing is caching. A simplistic implementation of caching the results of the api call would be:

$url = "http://developer.echonest.com/api/v4/artist/search?api_key=$APIkey&style=rap&results=10&start=$element_num&bucket=id:deezer&bucket=images&sort=familiarity-desc&format=xml";
$cacheFile = md5($url) . '.xml';

// If the cache file exists and is less than an hour old - use it.
if (file_exists($cacheFile) && filemtime($cacheFile) > (time() - 3600)) {
    $xml = file_get_contents($cacheFile);
// else download it
} else {
    $xml = file_get_contents($url);
    file_put_contents($cacheFile, $xml);
}
$xml = simplexml_load_file($cacheFile);

I.e. don't talk the the api on each request - talk to it once, and use the response for as long as you consider it appropriate/safe to consider valid (in the example above for 3600 seconds - or one hour).

This kind of logic is best encapsulated in a method.

DRY code execution

Less important compared to caching, the code in the question is 4 foreach loops which feed off each other - this is not an efficient way to write such logic. It's a good idea to apply do not repeat yourself to code as executed i.e. this:

foreach($echonest_xml->artists->artist as $artist){
    ...
    $full_deezer_id[] = $artist->foreign_ids->foreign_id->foreign_id;
}

foreach($full_deezer_id as $key => $value){ // <--
    ...
    $deezer_id[] = (string)$id[1];
}

foreach($deezer_id as $id_index => $id){ // <--
    ...
}

foreach($deezer_id as $key => $value){ // <--

Can be better written as

foreach($echonest_xml->artists->artist as $artist){
    processArtist($artist);
}

foreach($buidThis as $row) {

Therefore write the code to loop at most twice (but preferably just once), not four times on the same data.

Using only the logic in the question this would be e.g. the first two loops can easily be combined:

foreach($echonest_xml->artists->artist as $artist){
    preg_match('#deezer:artist:([A-Z,a-z,0-9]+)#', $artist->foreign_ids->foreign_id->foreign_id, $id);
    $deezer_id[] = (string)$id[1];
}

The same analysis should be applied to all code: try to limit the amount of iteration/recursion logic.

Collectives™ on Stack Overflow

PHP - is my code efficient?

2 Answers 2

Comments

Caching

DRY code execution

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Caching

DRY code execution

Comments

Your Answer

Sign up or log in

Post as a guest

Related