In PHP's Defense

Josh VanderLinden

Comments

So the other day I wrote up an article that sarcastically compared PHP and Python syntax a little. While I am completely serious when I say that I prefer Python's syntax a heck of a lot more than that of PHP, I thought it might be a good thing for me to demonstrate that the code I posted before could have been more appealing had it been thought out a little more. After a solid year of not really dealing with anything besides Python, I will share a feeble attempt at cleaning up/optimizing the PHP code.

Let's start with this snippet:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
 
<?php
$this->result=(isset($tmp["UMstatus"])?$tmp["UMstatus"]:"Error");
$this->resultcode=(isset($tmp["UMresult"])?$tmp["UMresult"]:"E");
$this->authcode=(isset($tmp["UMauthCode"])?$tmp["UMauthCode"]:"");
$this->refnum=(isset($tmp["UMrefNum"])?$tmp["UMrefNum"]:"");
$this->batch=(isset($tmp["UMbatch"])?$tmp["UMbatch"]:"");
$this->avs_result=(isset($tmp["UMavsResult"])?$tmp["UMavsResult"]:"");
$this->avs_result_code=(isset($tmp["UMavsResultCode"])?$tmp["UMavsResultCode"]:"");
$this->cvv2_result=(isset($tmp["UMcvv2Result"])?$tmp["UMcvv2Result"]:"");
$this->cvv2_result_code=(isset($tmp["UMcvv2ResultCode"])?$tmp["UMcvv2ResultCode"]:"");
$this->vpas_result_code=(isset($tmp["UMvpasResultCode"])?$tmp["UMvpasResultCode"]:"");
$this->convertedamount=(isset($tmp["UMconvertedAmount"])?$tmp["UMconvertedAmount"]:"");
$this->convertedamountcurrency=(isset($tmp["UMconvertedAmountCurrency"])?$tmp["UMconvertedAmountCurrency"]:"");
$this->conversionrate=(isset($tmp["UMconversionRate"])?$tmp["UMconversionRate"]:"");
$this->error=(isset($tmp["UMerror"])?$tmp["UMerror"]:"");
$this->errorcode=(isset($tmp["UMerrorcode"])?$tmp["UMerrorcode"]:"10132");
$this->custnum=(isset($tmp["UMcustnum"])?$tmp["UMcustnum"]:"");

$this->avs=(isset($tmp["UMavsResult"])?$tmp["UMavsResult"]:"");
$this->cvv2=(isset($tmp["UMcvv2Result"])?$tmp["UMcvv2Result"]:"");
?>

We see a lot of duplicate code in this chunk of code. The only thing that really changes much are the variable names and associative array keys. If we had defined a function that looked something like this...

1
2
3
4
5
 
<?php
function assign($member, $arr, $key, $default='') {
    $this->$member = isset($arr[$key]) ? $arr[$key] : $default;
}
?>

...things might just look a bit better. Let's see what the snippet might look like with this function defined in the same class:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
 
<?php
$this->assign("result", $tmp, "UMstatus", "Error");
$this->assign("resultcode", $tmp, "UMresult", "E");
$this->assign("authcode", $tmp, "UMauthCode");
$this->assign("refnum", $tmp, "UMrefNum");
$this->assign("batch", $tmp, "UMbatch");
$this->assign("avs_result", $tmp, "UMavsResult");
$this->assign("avs_result_code", $tmp, "UMavsResultCode");
$this->assign("cvv2_result", $tmp, "UMcvv2Result");
$this->assign("cvv2_result_code", $tmp, "UMcvv2ResultCode");
$this->assign("vpas_result_code", $tmp, "UMvpasResultCode");
$this->assign("convertedamount", $tmp, "UMconvertedAmount");
$this->assign("convertedamountcurrency", $tmp, "UMconvertedAmountCurrency");
$this->assign("conversionrate", $tmp, "UMconversionRate");
$this->assign("error", $tmp, "UMerror");
$this->assign("errorcode", $tmp, "UMerrorcode", "10132");
$this->assign("custnum", $tmp, "UMcustnum");

$this->assign("avs", $tmp, "UMavsResult");
$this->assign("cvv2", $tmp, "UMcvv2Result");
?>

In my opinion, this still isn't as appealing as the Python solution, but I'd take it over the original code. It's a lot easier to read. This may or may not be the best solution on any level of scrutiny--feel free to comment with any suggestions for ways to further improve things.

The second snippet from my original post could use a lot more help than the first one. I don't know who these guys are who wrote the PHP USA ePay module, but I think they could use a little assistance. No offense if you're reading this article--just some friendly constructive criticism. I would expect no less from anyone else who was examining my code and found ways to improve its efficiency.

Here's the original:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
 
<?php
switch(substr($ccnum,0,1))
{
    case 2: //enRoute - First four digits must be 2014 or 2149. Only valid length is 15 digits
        if((substr($ccnum,0,4) == "2014" || substr($ccnum,0,4) == "2149") && strlen($ccnum) == 15) return 20;
        break;
    case 3: //JCB - Um yuck, read the if statement below, and oh by the way 300 through 309 overlaps with diners club.  bummer.
        if((substr($ccnum,0,4) == "3088" || substr($ccnum,0,4) == "3096" || substr($ccnum,0,4) == "3112" || substr($ccnum,0,4) == "3158" || substr($ccnum,0,4) == "3337" ||
            (substr($ccnum,0,8) >= "35280000" ||substr($ccnum,0,8) <= "358999999")) && strlen($ccnum)==16)
        {
            return 28;
        } else {
            switch(substr($ccnum,1,1))
            {
                case 4:
                case 7: // American Express - First digit must be 3 and second digit 4 or 7. Only Valid length is 15
                    if(strlen($ccnum) == 15) return 3;
                    break;
                    case 0:
                case 6:
                case 8: //Diners Club/Carte Blanche - First digit must be 3 and second digit 0, 6 or 8. Only valid length is 14
                    if(strlen($ccnum) == 14) return 4;
                    break;
            }
        }
        break;
    case 4: // Visa - First digit must be a 4 and length must be either 13 or 16 digits.
        if(strlen($ccnum) == 13 || strlen($ccnum) == 16)
        {
            return 2;
        }
        break;

    case 5: // Mastercard - First digit must be a 5 and second digit must be int the range 1 to 5 inclusive. Only valid length is 16
        if((substr($ccnum,1,1) >=1 && substr($ccnum,1,1) <=5) && strlen($ccnum) == 16)
        {
            return 1;
        }
        break;
case 6: // Discover - First four digits must be 6011. Only valid length is 16 digits.
        if(substr($ccnum,0,4) == "6011" && strlen($ccnum) == 16) return 10;
}
?>

The first, and most obvious, improvement I would make to this code is to cram the substr($ccnum,0,4) junk into its own variable. It's used 8 different times up there. While substring operations might not be the most costly of functions out there, there's no need to repeatedly call the same function to get the same value that many times in the same block of code.

Similar to how I wrote the Python version, I would also throw the things that are repeatedly compared to the substr($ccnum,0,4) into an array and use the in_array function to increase readability. Oh, and consistent indentation (and not just because I like Python--it's good style to align things).

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
 
<?php
$four = substr($ccnum, 0, 4);
switch (substr($ccnum, 0, 1)) {
    case 2:
        /* enRoute - First four digits must be 2014 or 2149. Only valid
           length is 15 digits */
        if (in_array($four, array("2014", "2149")) && strlen($ccnum) == 15) return 20;
        break;
    case 3:
        /* JCB - Um yuck, read the if statement below, and oh by the way
           300 through 309 overlaps with diners club.  bummer. */
        if (in_array($four, array("3088", "3096", "3112", "3158", "3337")) ||
            in_array(substr($ccnum, 0, 8), array("35280000", "358999999")) &&
            strlen($ccnum) == 16) {
            return 28;
        } else {
            switch (substr($ccnum, 1, 1)) {
                case 4:
                case 7:
                    /* American Express - First digit must be 3 and second
                       digit 4 or 7. Only Valid length is 15 */
                    if(strlen($ccnum) == 15) return 3;
                    break;
                case 0:
                case 6:
                case 8:
                    /* Diners Club/Carte Blanche - First digit must be 3
                       and second digit 0, 6 or 8. Only valid length is 14 */
                    if(strlen($ccnum) == 14) return 4;
                    break;
            }
        }
        break;
    case 4:
        /* Visa - First digit must be a 4 and length must be either 13 or
           16 digits. */
        if (strlen($ccnum) == 13 || strlen($ccnum) == 16) {
            return 2;
        }
        break;
    case 5:
        /* Mastercard - First digit must be a 5 and second digit must be
           int the range 1 to 5 inclusive. Only valid length is 16 */
        if ($ccnum[1] >= 1 && $ccnum[1] <= 5 && strlen($ccnum) == 16) {
            return 1;
        }
        break;
    case 6:
        /* Discover - First four digits must be 6011. Only valid length
           is 16 digits. */
        if ($four == "6011" && strlen($ccnum) == 16) return 10;
}
?>

That just feels better to me. It should work exactly the same as the original snippet (though I admit I haven't tested it--don't even have PHP installed these days), but it just looks a heck of a lot better to me. Again, it might not be the most efficient way of accomplishing the desired task, but I consider these minor changes to make all the difference when you're required to maintain the code you wrote :)

You might notice that my version of the PHP is 10 lines longer than the original. That's mostly due to the fact that I try to respect the 80-character margin by wrapping lines before reaching that point. I believe this also adds to the pleasing appearance, but I realize that's more of a subjective thing these days.

Flame away folks!

Comments

Comments powered by Disqus