3
\$\begingroup\$

I'm working on this code and I can't find a better way to improve this code, can anyone improve it more? My PHP is pretty bad, this is the best I could do.

The code checks if iTunes link exists in the database if not search it on iTunes with API and if successfully found a link in itunes api it will be added in the database. If iTunes search is empty and iTunes row in the database is empty a string 'gol' will be saved in the MySQL and based on this string it will be rechecked in the future because Itunes has a 20 searches per minute limit. Hope i explained what is the code supposed to do.

if (empty($video['itunes'])) {
    //if mysql row $video['itunes'] is empty check the function ituneslink
    $itunecheck = ituneslink($mystrings);
    //ituneslink is a function that searches the itunes library for Movies/Music
    if (empty($itunecheck)) {
    //if ituneslink function is empty trigger the empty string itunesgol and
        $itunesgol = '';
        $smarty->assign('itunesgol', $itunesgol);
        itunes_add_item("gol", $video['uniq_id']);
    //add in mysql 'gol' that the itunescheck didn't found nothing on itunes 
    //search api, this will be useful for future rechecks
    }
} elseif($video['itunes'] != "gol") {
    // if row itunes is not 'gol' it means that itunescheck got the itunes link 
    //and succesfully added in database
    $itunecheck = $video['itunes'];
} else {
    $itunesgol = '';
    $smarty->assign('itunesgol', $itunesgol);
}


if ($video['itunes'] == 'gol') {
    //because itunes seach api has a limit of 20 req per minute i had to recheck
    //for this string if available because maybe the first search got 403 
    //fobiden because of to many queries
    $itunecheck = ituneslink($mystrings);
    itunes_add_item($itunecheck, $video['uniq_id']);
}

if (empty($video['itunes']) && !empty($itunecheck) && $video['itunes'] != "gol") {

    //NOW here i check if itunes is empty and itunescheck not empty and row 
    //itunes not 'gol'. If all of them comply it will be added in the database
    itunes_add_item($itunecheck, $video['uniq_id']);
}

The code works well, what i want is: to improve it, to shorten the code, to make it smoother, faster, it looks too messy

\$\endgroup\$
  • \$\begingroup\$ I believe they mean that the indentation on the comments makes the code very hard to read. Normally comments are indented at the same level as the code they are referring to. I agree and would add that many of the comments are useless and should be removed. For example you shouldn't explain what ituneslink() is when calling it. If you did that, you'd have the same comment every time you called it. If a reader wants to know what it does, they can look that function up. It would be better to use comments to explain why you're doing things and then only when necessary. \$\endgroup\$ – user1118321 May 31 '17 at 6:00
  • 2
    \$\begingroup\$ @BKSpurgeon please don't ask users to format and repost their code. If the formatting is messy, point it out in an answer. \$\endgroup\$ – janos May 31 '17 at 8:19
  • \$\begingroup\$ @user1118321 you've made a very good point, but it would have been better in an answer. \$\endgroup\$ – janos May 31 '17 at 8:20
  • \$\begingroup\$ functional-programming is a specific term that does not mean what you think it does. \$\endgroup\$ – 200_success May 31 '17 at 13:54
  • \$\begingroup\$ I think you need more context here. What is $video? What values need to go to smarty template? What is difference between $itunecheck and $itunesgol? What does 'gol' represent? It is really hard to follow your logic. \$\endgroup\$ – Mike Brant May 31 '17 at 15:17
1
\$\begingroup\$
if (empty($video['itunes']) && !empty($itunecheck) && $video['itunes'] != "gol") {

If $video['itunes'] is empty, then you already know that it does not equal "gol". So you could just say

if (empty($video['itunes']) && !empty($itunecheck)) {

And you would get the same effect.

Similarly, we could rewrite the whole thing as

if (empty($video['itunes'])) {
    //if mysql row $video['itunes'] is empty check the function ituneslink
    $itunecheck = ituneslink($mystrings);
    //ituneslink is a function that searches the itunes library for Movies/Music
    if (empty($itunecheck)) {
        //if ituneslink function is empty trigger the empty string itunesgol and
        $itunesgol = '';
        $smarty->assign('itunesgol', $itunesgol);
        itunes_add_item('gol', $video['uniq_id']);
        //add in mysql 'gol' that the itunescheck didn't find anything on itunes 
        //search api, this will be useful for future rechecks
    } else {
        //NOW here I check if itunes is empty and itunescheck not empty and row 
        //itunes not 'gol'. If all of them comply it will be added in the database
        itunes_add_item($itunecheck, $video['uniq_id']);
    }
} elseif($video['itunes'] == 'gol') {
    $itunesgol = '';
    $smarty->assign('itunesgol', $itunesgol);
    //because itunes search api has a limit of 20 req per minute I had to recheck
    //for this string if available because maybe the first search got 403 
    //forbidden because of too many queries
    $itunecheck = ituneslink($mystrings);
    itunes_add_item($itunecheck, $video['uniq_id']);
} else {
    // if row itunes is not 'gol' it means that itunescheck got the itunes link 
    //and successfully added in database
    $itunecheck = $video['itunes'];
}

This gets rid of the redundant if statements, adding to the original blocks instead.

Or

if (empty($video['itunes']) || ($video['itunes'] == 'gol')) {
    // if no cached result, or cached as `gol`
    // fetch new results from iTunes
    $itunecheck = ituneslink($mystrings);
    if (empty($itunecheck)) {
        $itunesgol = '';
        $smarty->assign('itunesgol', $itunesgol);
        itunes_add_item('gol', $video['uniq_id']);
    } else {
        itunes_add_item($itunecheck, $video['uniq_id']);
    }
} else {
    // otherwise use cached result
    $itunecheck = $video['itunes'];
}

This changes the behavior slightly. It doesn't assume that a second iTunes search will always return results. Instead, it treats a cached failure the same as no result in cache.

But if we do that, we can go ahead and just say

if (empty($video['itunes'])) {
    // if no cached result
    // fetch new results from iTunes
    $itunecheck = ituneslink($mystrings);
    if (empty($itunecheck)) {
        $itunesgol = '';
        $smarty->assign('itunesgol', $itunesgol);
    } else {
        itunes_add_item($itunecheck, $video['uniq_id']);
    }
} else {
    // otherwise use cached result
    $itunecheck = $video['itunes'];
}

This removes the concept of caching failures. Instead, we'll just generate a cache miss next time.

You could also go the other direction and add more complexity such that you sometimes redo a 'gol' and sometimes don't. Or whatever. But as things are written, there doesn't seem to be any reason to have two cases with mostly the same behavior.

It is of course possible that I'm missing something. You don't include definitions for ituneslink, itunes_add_item, and $smarty->assign, so there could be special stuff happening there. Or prior to this code in setting $video['itunes']. Or after this code that uses $itunesgol or $itunecheck. But from what I see of this code, this seems to do mostly the same thing as the original.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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