CODEWITHSUNDEEP
php
Mike Jones
Mike Jones

Reputation: 13

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

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");


?>

Upvotes: 0

Views: 191

Answers (5)

t1gor
t1gor

Reputation: 1292

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! :)

Upvotes: 1

Pekka
Pekka

Reputation: 449783

$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.

Upvotes: 2

Your Common Sense
Your Common Sense

Reputation: 157989

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"]

Upvotes: -1

David Knell
David Knell

Reputation: 764

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
}

Upvotes: 0

Androme
Androme

Reputation: 2449

First here are some tips.

  1. 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.
  2. 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.
  3. Remember to use mysql_real_escape_string with you are pasting variables into a sql string, to safeguard againg sql injections.
  4. When you have a if where one of the condition do nothing.. leave it out
  5. 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");

Upvotes: 0

Related Questions