Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

enter image description here

  1. Delete all matching ID
  2. Insert new data

    function add_date($id,$date) {

    mysql_query("DELETE FROM wp_opening_date WHERE Id='$id'");
    $dates = explode(",",$date);
    foreach ($dates as $date) { 
        $date = explode("/",$date);
        $d    = $date[0];
        $m    = $date[1];
        $y    = $date[2];   
        mysql_query("INSERT INTO wp_opening_date (Id, D, M, Y) VALUES ('$id','$d', '$m', '$y')");   
    }
    mysql_close();
    

    }

And i using function like this

$id = '277';
$dates = '01/10/2012,28/11/2012,27/11/2012,29/11/2012';
add_date($id,$dates);

Review my code + idea pls

The idea i will searching Ids by month or year or date

share|improve this question

2 Answers 2

up vote 2 down vote accepted

Well, there's nothing wrong as far as I can see here. The only thing I would check is that there is a suitable index on the Id field in database, so the delete query can be performed relatively quick.

Another thing to consider is the size of your table. If you have lots of records in there, it can take considerable time to remove and re-add records again (due to indexing and re-indexing upon each delete/insert). In such case, I would simply add a flag to the database, marking those old dates as deleted and simply add new ones at the end of the table.

Yet another thing that comes to my mind now is that you can gain even more performance-wise if you grouped those single inserts like so:

$vals = array();
foreach ($dates as $date) { 
    $date = explode("/",$date);
    $d    = $date[0];
    $m    = $date[1];
    $y    = $date[2];
    $vals[] = "('$id', '$d', '$m', '$y')";
}
mysql_query("INSERT INTO wp_opening_date (Id, D, M, Y) VALUES ".implode(',', $vals));

At last, please do not use mysql_* functions, they are now deprecated. See the PHP manual for mysql_query for more info.

share|improve this answer
    
Thanks so much, but where is DELETE ? , I will delete all first and add dates –  l2aelba Nov 29 '12 at 8:14
    
I only included the updated part of code - the delete and function definition can stay where it is ;) –  Zathrus Writer Nov 29 '12 at 8:15
    
Ok , thanks man :D –  l2aelba Nov 29 '12 at 8:16
    
Can you edit your code to full function ? , I will sure 100% that i learn from your is right :P –  l2aelba Nov 29 '12 at 8:17
1  
sry, my bad... they are not deprecated yet, however they will be one day - so if you're making a new PHP application, it's best to choose one of the alternatives - see links in the warning message on mysql_query manual page –  Zathrus Writer Nov 29 '12 at 8:25

On first glance, I see a few problem areas with the code that could help with some of the pitfalls of the function you posted.

  1. Use of mysql functions are deprecated. Try to use the PDO driver if at all possible through use of the mysql driver they offer. The PDO driver is also much more robust and will allow for this function to not just be coupled to mysql.

  2. The way you are getting your date is VERY brittle. To assume that the date is in a format of XX/XX/XXXX is a poor assumption. Try using something that you can specify format to get the individual piece of the date. For example:

    $dateObject = new \DateTime($date);
    $d = $dateObject->format('d');
    $m = $dateObject->format('m');
    $y = $dateObject->format('Y');
    
  3. Your parameters for the function are passed straight through to query. This makes you vulnerable to sql injection attacks in some cases. ALWAYS prepare your parameters. So we have;

    mysql_query("DELETE FROM wp_opening_date WHERE Id=:ID");
    
  4. The final problem is one of testability. How can an automated test be run against this function to make sure it performs correctly. I think a better way would be to inject the connection object rather than to get it inside the function. This would allow you to inject dependencies and foster testability.

To wrap it all up we have a final function of:

    function addDates($id, $date)
    {
        try {
            $conn   = new \PDO(<Connection Info>);

            $delete = "DELETE FROM wp_opening_date WHERE Id = :ID";
            $stmt   = $conn->prepare($delete);
            $stmt->bindParam(':ID', $id);
            $stmt->execute();

            $vals  = array();
            $dates = explode(",", $date);
            foreach ($dates as $date) {
                $dateObject = new \DateTime($date);
                $d = $dateObject->format('d');
                $m = $dateObject->format('m');
                $y = $dateObject->format('Y');

                $vals[] = "('$id', '$d', '$m', '$y')";
            }

            $insert = "INSERT INTO wp_opening_date (Id, D, M, Y) VALUES " . implode(',', $vals);
            $stmt = $conn->prepare($insert);
            $stmt->execute();
        }
        catch(\PDOException $e) {
            < Do your failure logic here >
        }
    }

Or something along those lines....

Hope this helps!

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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