编码改进[关闭]

I need to check if a monthly contract exist and if is signed. I would improve the code because it seems too heavy

It's time to switch to PDO and maybe i can rewrite some things to speed up script

Basically I have a table with 2 columns 1st column have date reference, 2013/jan, 2013/fev ... and 2nd column display check if document exist and have signature.

Here my code. There is a way to improve?

// QUERY DB
// Get contract
$contract = array();
$query1 = mysql_query("SELECT * FROM contracts
                       WHERE ic_id='28'");
while ($row = mysql_fetch_assoc($query1)) {
    $contract[] = $row['month'];          
}

$signature = array();
$query1 = mysql_query("SELECT * FROM contracts
                       WHERE ic_id='28'");
while ($row = mysql_fetch_assoc($query1)) {
    $signature[] = $row['sign'];          
}

// Get START AND END DATE

$startYear =  '2012';
$startMonth = '12';

$endYear =  '2013';
$endMonth = '12';

$startDate = strtotime("$startYear/$startMonth/01");
$endDate   = strtotime("$endYear/$endMonth/01");

$currentDate = $endDate;

RESULTS TABLE

<tbody>
    <?php
        while ($currentDate >= $startDate) {
        $foo = date('Y-m',$currentDate);   
        $currentDate = strtotime( date('Y/m/01/',$currentDate).' -1 month');
    ?>       
    <tr class="grade">
        <td>    
           <?php   echo $foo; ?>
        </td>

        <td>
        <?php 

        if (in_array($foo, $contract)) { 
        echo "<img src='images/ok.png';
        } else {
        echo "<img src='images/ko.png'/>";   
        }

        if (in_array($foo, $signature)) { 
        echo "<img src='imagens/ok.png'/>";
        } else {
        echo "<img src='imagens/sign.png'/>";   
        }

        ?>
        </td>
    </tr>

    <?php
    $i++;
    }
    ?>                    
</tbody>

Connect to PDO.

 <?php
 $contract = array();
 $signature = array();
 $db = new PDO('mysql:host=localhost;dbname=<SOMEDB>', '<USERNAME>', 'PASSWORD');
 $st = $db->prepare("SELECT month ,sign FROM contracts   WHERE ic_id=?");
 $st->execute(array('28'));

 foreach($st as $row)
 {
   $contract[] = $row['month']; 
   $signature[] = $row['sign'];
 }
 ?>

Why do you use variables where there are contant information? For example in start and end date. As you know mysql_* are depracated so you should move to PDO ASAP. Naming variables as $var1, $var2 is really bad convention.

Check these links:

  1. Zend Conventions
  2. PEAR standards

As I wrote above

$startYear =  '2012';
$startMonth = '12';

$endYear =  '2013';
$endMonth = '12';

$startDate = strtotime("$startYear/$startMonth/01");
$endDate   = strtotime("$endYear/$endMonth/01");

these variables seem to be constant so use constant string.

$startDate = strtotime("2012/12/01");
$endDate   = strtotime("2013/12/01");

Moreover, you select all columns in query but you use only 2, I've changed your query.

This part of code

<?php 

    if (in_array($foo, $contract)) { 
    echo "<img src='images/ok.png';
    } else {
    echo "<img src='images/ko.png'/>";   
    }

    if (in_array($foo, $signature)) { 
    echo "<img src='imagens/ok.png'/>";
    } else {
    echo "<img src='imagens/sign.png'/>";   
    }

    ?>

can be changed to

<?php 

    echo "<img src='images/".(in_array($foo, $contract) ? "ok" : "ko").".png";
    echo "<img src='imagens/".(in_array($foo, $signature) ? "ok" : "ko").".png";
?>

less code :)

echo also has a shortcut syntax, where you can immediately follow the opening tag with an equals sign. You can change this:

<?php   echo $foo; ?>

to this

<?=$foo?>

the last tip is to use pre incrementation because it is faster. If you don't need post incrementaction then use pre!

change $i++ to ++$i;

The first thing that comes to mind is this duplicate SQL query...

Existing

$contract = array();
$query1 = mysql_query("SELECT * FROM contracts
                       WHERE ic_id='28'");
while ($row = mysql_fetch_assoc($query1)) {
    $contract[] = $row['month'];          
}

$signature = array();
$query1 = mysql_query("SELECT * FROM contracts
                       WHERE ic_id='28'");
while ($row = mysql_fetch_assoc($query1)) {
    $signature[] = $row['sign'];          
}

Updated

$contract = array();
$signature = array();
$query1 = mysql_query("SELECT month, sign FROM contracts
                       WHERE ic_id='28'");
while ($row = mysql_fetch_assoc($query1)) {
    $contract[] = $row['month'];   
    $signature[] = $row['sign'];       
}

Changes

  • Removed the duplicate SQL queries. There is no need to query & loop over the same information twice.
  • Updates SQL to only request month and sign columns, since that's all your using the extra is wasted memory / processing.