How can I make my code better and/or more efficient

Question

I'm trying to improve my PHP programming skills, can anyone give me any tips or direction based on this code that I wrote?

<?php

include("db.php");
include("function.php");

//variables
$number = htmlspecialchars($_POST['num']);
$date = date("Y-m-d");

//validate phone number
if (strlen($_POST['num']) != 12){
print "Invalid Phone Number.";
die();
}

//check how many times the number was called today

    $callstoday = mysql_query("
    SELECT number
    FROM numbers 
    WHERE number = '$number'
    AND date 
    LIKE '$date%'")
    or die(mysql_error());
    $callstotal = mysql_num_rows($callstoday);



//cant do more than 5 calls

 if ($callstotal < 5){
  //do nothing
 }else{
  print "Not Allowed";
 die();
 }





//break up the number in 3 parts
$bits = explode("-", $number);
$data = get_carrier("http://site.com/?action=carrierlookup&p1=".$bits[0]."&p2=".$bits[1]."&p3=".$bites[2]."&iecache=0");


//check when they want to call

if ($_POST['when'] == 'now' ){
$when = "0";
}elseif($_POST['when'] == 'secs'){
$when = "30";
}elseif($_POST['when'] == 'minute'){
$when = "60";
}elseif($_POST['when'] == '2minute'){
$when = "120";
}elseif($_POST['when'] == '5minute'){
$when = "300";
}

//check for carrier
if(strstr($data, 'Cingular')){
$carrier = "AT&T";
}elseif(strstr($data, 'Sprint')){
$carrier = "Sprint";
}elseif(strstr($data, 'Verzion')){
$carrier = "Verzion";
}elseif(strstr($data, 'T-Mobile')){
$carrier = "T-Mobile";
}elseif(strstr($data, 'Boost')){
$carrier = "Boost Mobile";
}elseif(strstr($data, 'Cricket')){
$carrier = "Cricket";
}elseif(strstr($data, 'Alltel')){
$carrier = "Alltel";
}elseif(strstr($data, 'Unable')){
$carrier = "Unknown Carrier";
}




//inset number and carrier into database.
mysql_query("INSERT INTO numbers (number, carrier)
VALUES ('$number', '$carrier')");
print "success";
mysql_close($con);


//call out to the number
$strippednumber = str_replace("-", "", $number);
$call = call("http://domain.com");


?>

Pekka · Answer 1 · 2010-08-15T08:29:13.670

2

$number = htmlspecialchars($_POST['num']);

will not prevent possible SQL injections. You need to add a

 $number = mysql_real_escape_string($number);

The $_POST["when"] check would be better off in an array check.

$whens = array("now" => "0", "secs" => "30".....);
if (array_key_exists($_POST['when'], $whens)) 
 $when = $whens[$_POST['when']];

same goes for the carrier check:

 $carriers = array("Cingular" => "AT&T", "Sprint" => "Sprint" .....);

 foreach ($carriers as $key => $value)
   if (strstr($data, $key))
     {
      $carrier = $value;
      break;
     }

You may want to add checks for if one of the POST variables is not set.

edited Aug 15 '10 at 08:29

answered Aug 15 '10 at 07:40

Pekka

442,112
142
972
1,088

htmlspecialchars will not prevent injection – Your Common Sense Aug 15 '10 at 07:45
You should use mysql_real_escape_string for injection prevention. – Androme Aug 15 '10 at 07:49
and I see nothing better in switch statement. it is less readable than plain and direct if statements. Though the same array approach as below would be the best. – Your Common Sense Aug 15 '10 at 07:50
@Col re injection prevention: true, corrected. Re using an array instead of switch: Also true, didn't think of that. Changed. – Pekka Aug 15 '10 at 07:59
Switching from if/switch to arrays dose not give you a preformance boost, easyer to read yes, but it caust a little more cpu time. – Androme Aug 15 '10 at 08:04
@DoomStone so what if it takes three milliseconds longer? Code readability is *way* more important than micro-optimization. – Pekka Aug 15 '10 at 08:07
I do not find find a problem with that, i would also do it that way :D I just wanted to point out that is it not faster! – Androme Aug 15 '10 at 08:09
Pekka it is not your day today :) $when = $whens[$_POST['when']]; – Your Common Sense Aug 15 '10 at 08:25
@Col it's still early in the morning, I'm on my second coffee :) Cheers. – Pekka Aug 15 '10 at 08:29

score 1 · Answer 2 · answered Nov 23 '12 at 14:12

include("db.php");
include("function.php");

Better use require_once('file').

$number = htmlspecialchars($_POST['num']);

I'd use (int)$_POST['num'] - prevents of any kind of surprises.

 if ($callstotal < 5){
  //do nothing
 }else{
  print "Not Allowed";
 die();
 }

"//do nothing" isn't really the best way. Do it like this:

 if ($callstotal >= 5){
  print "Not Allowed";
  die();
 }

Also I agree about this one:

$carriers = array("Cingular" => "AT&T", "Sprint" => "Sprint" .....);

All the best! :)

Androme · Answer 3 · 2010-08-15T08:01:54.577

0

First here are some tips.

use ' instead of " for strings without variables, when you are using " will php look for variables inside the string, with ' will it just paste it out. This will save you some cpu time.
When working with mysql, and you just want to count the result is doing a simple count(0) much faster then fetching all the rows and the count them.
Remember to use mysql_real_escape_string with you are pasting variables into a sql string, to safeguard againg sql injections.
When you have a if where one of the condition do nothing.. leave it out
When you have files that only contain php code are there no reason to end the php tag "?>" i will only cause you problems with looking for that god dam new line og space that comes from some where.

There are not much that can be optimized in the code you gave, mostly readability. I have updated your script abit, fixed som injection problems and updated teh readability a bit.

But if you realy want to take you php programming to a whole new level, take a look at classes first, and when you are comfortable with them, try MVC. I can recommend the Zend Framework.

<?php
include("db.php");
include("function.php");

//variables
$number = htmlspecialchars($_POST['num']);
$date = date("Y-m-d");

//validate phone number
if (strlen($_POST['num']) != 12){
print "Invalid Phone Number.";
die();
}

//check how many times the number was called today

$callstoday = mysql_query('
    SELECT count(0) as `count`
    FROM numbers 
    WHERE number = '.mysql_real_escape_string($number).'
    AND date 
    LIKE \''.mysql_real_escape_string($date).'%\'')
or die(mysql_error());
$callstoday = $callstoday[0]['count'];


//cant do more than 5 calls
if ($callstotal >= 5){
    print "Not Allowed";
    die();
}

//break up the number in 3 parts
$bits = explode('-', $number);
$data = get_carrier('http://site.com/?action=carrierlookup&p1='.$bits[0].'&p2='.$bits[1].'&p3='.$bites[2].'&iecache=0');


//check when they want to call
switch($_POST['when'])
{
    case 'now':
        $when = 0;
        break;
    case 'secs':
        $when = 30;
        break;
    case 'minute':
        $when = 60;
        break;
    case '2minute':
        $when = 120;
        break;
    case '5minute':
        $when = 300;
        break;
}
//check for carrier
if(strstr($data, 'Cingular'))
    $carrier = "AT&T";
elseif(strstr($data, 'Sprint'))
    $carrier = "Sprint";
elseif(strstr($data, 'Verzion'))
    $carrier = "Verzion";
elseif(strstr($data, 'T-Mobile'))
    $carrier = "T-Mobile";
elseif(strstr($data, 'Boost'))
    $carrier = "Boost Mobile";
elseif(strstr($data, 'Cricket'))
    $carrier = "Cricket";
elseif(strstr($data, 'Alltel'))
    $carrier = "Alltel";
elseif(strstr($data, 'Unable'))
    $carrier = "Unknown Carrier";



//inset number and carrier into database.
mysql_query("INSERT INTO numbers (number, carrier)
VALUES (\''.mysql_real_escape_string($number).'\', \''.mysql_real_escape_string($carrier).'\')");
print "success";
mysql_close($con);


//call out to the number
$strippednumber = str_replace("-", "", $number);
$call = call("http://domain.com");

edited Aug 15 '10 at 08:01

answered Aug 15 '10 at 07:48

Androme

2,399
4
43
82

One who upvoted *this*, didn't even bother to read the code. Not to mention silly and misleading "advises". – Your Common Sense Aug 15 '10 at 07:57
I can't see any real need to surround $date with mysql_real_escape_string. – David Knell Aug 15 '10 at 08:00
You are right, using mysql_real_escape_string on $date is a bit of overkill :D I just always use it on all varaibles as a rule! – Androme Aug 15 '10 at 08:01
@David actually - yes, you are right. But it should be a habit. One should not think of a variable contents or source. It is like prepared statements: would you decide to use it or not for the each variable? No, you would use it for them all, no matter of it source or contents. Same here. Just escape everything you put into query in quotes. As a rule. – Your Common Sense Aug 15 '10 at 08:03
Col. Shrapnel: could you please explain what you find silly and misleading about my advises? And why don't you think i read the code? – Androme Aug 15 '10 at 08:07
1 is just stupid. and as any stupid assumption, it led you to the broken code when you tried to implement it. 2 is truth in general but doesn't matter here. 3 is not really truth as it works not with every variable you paste. mysql_real_escape_string alone is not a safeguard against sql injections. 5 is a trifle thing, not worth mentioning. especially if you are talking of a script that already outputs. there is a shitload to improve in this script actually, not "not much" as you say – Your Common Sense Aug 15 '10 at 08:15
1: Do some reading, you can get about 5-10% more performance from then depending on the amount of strings in your code! 2: Yes it dose, we dose a select without using anything else then the count. 3: No it is not a complete safeguade, but it is a good step in the right direction! 4: It might not be importet to this script, but as he said he want to be better, and when you start using class files will this be a pain, and this is a good thing to learn.. Please learn som advance php befor you go on bashing spree – Androme Aug 15 '10 at 08:24
lol. I have read a shitload of these stupid articles, written by brainless idiots. They are all do not understand what are they talking about. There are no measurable difference. Not even a microoptimization. It seems you are not able to learn, that saddens me. – Your Common Sense Aug 15 '10 at 08:31
I agree that 1) is unnecessary micro-optimization. 2) is a good point though IMO: A count() is bound to be easier on the database server and good practice. 4) and 5) I find okay to mention, the OP *is* asking about tips how to improve his code. – Pekka Aug 15 '10 at 08:35
@Pekka as a matter of fact, 5 is irrelevant to this very code. This is actually a bunch of mindless prayers, rather than real improvings to this particular code. – Your Common Sense Aug 15 '10 at 08:42
We can discuss 1, till the very end, there are different opinions on this... But 5: is in no way irrelevant. The only reason a person can see it as irrelevant is if you only are doing VERRY low end php code with a very little number of includes. When you get up with the big boys where we have hundreds of includes and classes that work with header and seasons will you see why i gave this advice! – Androme Aug 15 '10 at 08:48
http://stackoverflow.com/questions/3486740/ is a fresh and perfect example of that widespread rumor "Remember to use mysql_real_escape_string with you are pasting variables into a sql string" – Your Common Sense Aug 15 '10 at 09:07

David Knell · Answer 4 · 2010-08-15T08:05:15.520

Three further things:

Verizon is spelt like that, not Verzion ;-)
Your SQL might be more efficiently written as date>=CUR_DATE() AND date<DATE_ADD(CUR_DATE(), INTERVAL 1 DAY) if your date column is a DATETIME - the select as written might require the database to convert each date to a string to do the comparison, unless its query optimizer recognises this sort of pattern.
You could better validate $num against a suitable regex, rather than just checking that it's 12 characters -as a side effect, this will also protect against SQL injection attacks:

if (!preg_match("/^[0-9]{3}-[0-9]{3}-[0-9]{4}$/", $num)) {
  // Invalid number - fail
}

score -1 · Answer 5 · answered Aug 15 '10 at 08:09

-1

I see no use of $when variable here. but it looks like you have to set it up already in the HTML form:

<option value ="0">now</option>
<option value ="30">secs</option>
...

will give you desired number already in the $_POST["when"]

answered Aug 15 '10 at 08:09

Your Common Sense

156,878
40
214
345

He still ned to validate the varaible in the script, so in the end will this be just the same, just with '30' instead of 'secs'. Totaly point less – Androme Aug 15 '10 at 08:27

How can I make my code better and/or more efficient

5 Answers5