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.

Could someone help me on how to eliminate some nested blocks or improve this code? I am concerned this will slow down my site dramatically.

function dispalyEvent($weekNr, $week, $year){
    echo "<p>";

    $gendate = new DateTime();
    $gendate->setISODate($year,$week,$weekNr);
    $event_query = mysql_query("SELECT * FROM calendar ORDER BY starttime");

    //Go through all event in the database
    while($event = mysql_fetch_array($event_query)) {   

    //Create a range for starting date and ending date
    $date1 = new DateTime($event['startyear'].$event['startmonth'].$event['startdate']);
    $date2 = new DateTime($event['endyear'].$event['endmonth'].$event['enddate']);
    $date2->modify('+1 day');

    $period = new DatePeriod($date1, new DateInterval('P1D'), $date2);

     $title = $event['title'];
     $name = $event['name'];

        $recur_query = mysql_query("SELECT * FROM recur WHERE title = '$title' AND name = '$name'");
        $recur = mysql_fetch_array($recur_query);

        $recurring = $recur['type'];

            //Find day of starting recurring event and ending day  
            if (!$recurring == "None"){
                $starttime = explode("/",$recur['startdate']); 
                $startdate = new DateTime();
                $startdate->setDate($starttime[2], $starttime[0], $starttime[0]);

                $endtime = explode("/",$recur['enddate']); 
                $enddate = new DateTime();
                $enddate->setDate($endtime[2], $endtime[0], $endtime[0]);

            }
            else {
                $startdate = new DateTime();
                $enddate = new DateTime();
            }

            //Put the dates in integer to find if it is out of range
            $displaydate = intval($gendate->format("Ymd"));
            $startdate = intval($startdate->format("Ymd"));
            $enddate = intval($enddate->format("Ymd"));
            settype($displaydate, "integer");
            settype($startdate, "integer");
            settype($enddate, "integer");

        //Go through each date in the range
        foreach ($period as $savedDate) {   
            //Check if the Item is Approved
            if ($event['Approved'] == "Approved"){

                switch($recurring){

                    Case 'None':

                        //If the date in the range is the same as the displaydate
                        if ($gendate->format("Y-m-d") == $savedDate->format('Y-m-d')){

                            //Create event
                            renderEvent($event['ad'], $event['starttime'], $event['title'], $event['endtime'], $event['location'], $event['address'], $event['price'], $event['description']);
                        }
                    break 1;

                    Case 'Daily':

                        //Check margin between start and end date of recurring event
                        if ($displaydate > $startdate and !$displaydate < $enddate){

                            //Check if the day number is the same
                            if ($recur['day']-1 ==  $gendate->format("w")){

                                //Create event
                                renderEvent($event['ad'], $event['starttime'], $event['title'], $event['endtime'], $event['location'], $event['address'], $event['price'], $event['description']);
                            }     
                        }
                    break 1;

                    Case 'Weekly':
                        //Check margin between start and end date of recurring event
                        if ($displaydate > $startdate and !$displaydate < $enddate){

                            //Find the amount of weeks between two dates
                            $weekRange = datediffInWeeks($recur['startdate'], $recur['enddate']);

                            //Round down to the possible amount to display
                            $weeks = ceil($weekRange / $recur['day']);

                            //Returns the week cuurent week to display
                            $currentWeek = $gendate->format("W");

                            //Loop for every #(1, 2, 3, 4) of weeks
                            for ($n=0; $n<$weeks; $n++) {

                                //Display event if weeks are the same
                                if ($n == $currentWeek) {

                                    //Put days in array
                                    $days = explode(",",$recur['weekday']);

                                    //If number day of the week is the same display event
                                    foreach ($days as $day) {

                                        //Check if the day number is the same
                                        if ($day == $gendate->format("w")) {

                                                //Create event
                                                renderEvent($event['ad'], $event['starttime'], $event['title'], $event['endtime'], $event['location'], $event['address'], $event['price'], $event['description']);
                                        }
                                    }
                                }   
                            }
                        }
                    break 1;
                }
            }
        }  
    }
    echo "</p>";
}
share|improve this question
    
Try to avoid nested if block and use ternary operator. Move code in functions. –  sunil Mar 31 at 6:39
    
Explain the purpose of this function. Is it called for each day on the calendar to render events for that day? –  Bob65536 Mar 31 at 8:24
    
While I agree with the functions part of your comment, I don't agree with using ternaries to avoid nesting blocks. if-else branches don't introduce a new block-scope, and the PHP ternary is fundamentally flawed. Besides, the OP's code is messy as it is, and there are issues with operator precedence all over. Ternaries will only add confusion and be a new source of bugs at this point. That's just bad advice in this case... –  Elias Van Ootegem Mar 31 at 11:47
add comment

2 Answers 2

up vote 8 down vote accepted

Ok, the following review may seem blunt or harsh, but please, try to keep in mind that this is in order to help. I'm not trying to hurt or mock anyone, but in order for code-review to be as effective as it ought to be, I'll have to be brutal.

If you haven't read it already, the help-section asks you post working code. bug-riddled code isn't subject to review yet, it has to be debugged first.
It is possible you aren't aware of it, and that you may think your code works, when really it doesn't. Well, not as you expect it to, at least. I know it feels banal and tedious, closely looking at the operator precedence table doesn't do any harm. Quite the opposite, in fact. You'll soon find out why Both David Harkness and myself mention potenial bugs or unexpected behaviour with expressions like:

if (!$recurring == "None")
//and
if ($displaydate > $startdate and !$displaydate < $enddate)

And as a last point in this foreword to what is already a hefty answer, I would like to strongly suggest you change your php.ini settings for the error_reporting and set display_errors to true, one, or stdout, depending on your PHP version.
The error_reporting's default value is likely to be E_ALL & ~E_NOTICE & ~E_STRICT & ~E_DEPRECATED, while debugging, it's best to set it to E_ALL | E_STRICT, or call error_reporting(-1); in your script.

As I have done before, I'll walk through your code line by line, offering advice and the reasoning behind my criticism. At the end, I'll add an example of code you could end up with if you decide to take my recommendations to heart.
Update: I did not add a code example as there are simply too many unknowns to deal with, and that any example would basically end up being a total re-write of your code, which isn't my job, and is of little educational use to you. Instead, just to make it 100% clear, however blunt or harsh this answer may seem here's a meta-post on why I consider it necessary for code-review to be tough
Now, without further ado, let's get too it:

function dispalyEvent($weekNr, $week, $year){

Yes, I have some criticisms about the very first line of code you posted already. Ok, a function displayEvent, that expects 3 arguments. All three have to do with time. But if you need variables that tell you something about time, why not ask of the user (caller) to pass a DateTime class from the off?

function displayEvent(DateTime $date)
{

Now this tells the user of your code that he's expected to pass a DateTime instance as an argument. It reduces the number of arguments from 3 to 1, and allows for type-hints. As we'll see in a second, this also saves you the bother of creating the DateTime instances inside the function. The advantage of that is that, if the caller already has a DateTime instance, he can simply pass that object, and not call methods to get the year, week and weekNr values, which are only being used to re-construct the same DateTime instance all over.

Onwards:

    echo "<p>";

Don't echo in a function. A function returns. The caller of your function may then choose to echo the return value, or may store it somewhere else. Having a function echo something puts the user of your code in a tight spot: calling this function means he can't set the headers, can't use this function to retrieve data and present it in a way he wants to. Just create a variable $outString = '';, and return that at the end.

    $gendate = new DateTime();
    $gendate->setISODate($year,$week,$weekNr);

As I said before: this code can be made redundant simply by changing the function's signature to expect a DateTime instance from the off

    $event_query = mysql_query("SELECT * FROM calendar ORDER BY starttime");

    //Go through all event in the database
    while($event = mysql_fetch_array($event_query)) {

Please, please, please stop using the deprecated mysql_* extension. Switch to PDO or mysqli_* instead. Henceforth I'll be using PDO.
And as a rule of thumb, or even personal mantra: Avoid SELECT * queries whenever you can. Select what you need, and how you need it. You haven't done that last bit at all, judging by the next snippet of code:

    //Create a range for starting date and ending date
    $date1 = new DateTime($event['startyear'].$event['startmonth'].$event['startdate']);
    $date2 = new DateTime($event['endyear'].$event['endmonth'].$event['enddate']);
    $date2->modify('+1 day');

Why not select these dates like so:

SELECT CONCAT_WS('-', startyear, startmonth, startdate) AS date1

That way, you'll be able to write:

$date1 = new DateTime($event['date1']);

That's just, I think you'll agree, a hell of a lot cleaner. Anyway, back to the code:

    $period = new DatePeriod($date1, new DateInterval('P1D'), $date2);

     $title = $event['title'];
     $name = $event['name'];

Why bother assigning individual variables, you have an associative array, what's wrong with that? An assoc array is a data structure that holds together all related data anyway. This data clearly belongs together, why not keep it together in that array?
We'll get to the DatePeriod business in a moment, for now, let's carry on:

$recur_query = mysql_query("SELECT * FROM recur WHERE title = '$title' AND name = '$name'");
$recur = mysql_fetch_array($recur_query);
$recurring = $recur['type'];
//Find day of starting recurring event and ending day  
if (!$recurring == "None"){
    $starttime = explode("/",$recur['startdate']); 
    $startdate = new DateTime();
    $startdate->setDate($starttime[2], $starttime[0], $starttime[0]);
    $endtime = explode("/",$recur['enddate']); 
    $enddate = new DateTime();
    $enddate->setDate($endtime[2], $endtime[0], $endtime[0]);
}
else {
    $startdate = new DateTime();
    $enddate = new DateTime();
}

Ok, you may have noticed I fixed the indentation. Seriously, indentation is important. For your sake and ours. Stay consistent and try to adhere to the standard as much as you can.
Anyway: This code basically queries the same DB for, pretty much, the same data over and over again. Of course, the where clause is different every time, but what you're doing is sending a string to MySQL, who then parses and compiles the query and then fetches the data.

A prepared statement can be sent to the DB once, to be compiled, optimized (and in many cases, a lot of the data is even pre-fetched), and you can then send the values that are to be placed in the WHERE clause whenever you need that query to be executed. This saves the DB server a lot of work, saves time and is more secure. You're just stringing $name and $title in the query, for example. Completely oblivious to the fact that There could be a name like "O'Connor" assigned to $name. Resulting in the Query:

SELECT * FROM recur WHERE title = 'foobar' AND name = 'O'Connor'

Which will cause problems. And what if Bobby Tables pays a visit?
On the DateTime things, I can only say: Why explode? Why not simply write:

$recur['startdate'] = new DateTime($recur['startdate']);

DateTime does a great job at "guessing" the format, but if you wish not to rely on this, you can always choose to specify the format yourself:

$recur['startdate'] = DateTime::createFromFormat(
    'd/m/Y',
    $recur['startdate']
);

Anyway, let's continue:

//Put the dates in integer to find if it is out of range
$displaydate = intval($gendate->format("Ymd"));
$startdate = intval($startdate->format("Ymd"));
$enddate = intval($enddate->format("Ymd"));
settype($displaydate, "integer");
settype($startdate, "integer");
settype($enddate, "integer");

DRY, Don't Repeat Yourself. You are calling the intval function. Look at the return type:

int intval ( mixed $var [, int $base = 10 ] )
// \---> returns an INT

Why, then are you calling settype? It's pretty safe to say you're calling settype on an int already. Even if you're not, why not cast? A cast saves the overhead of a function call:

$displaydate = (int) $gendate->format("Ymd");

That's all there is too it, and you've saved yourself the bother of 2 function calls.
Moving on:

//Go through each date in the range
foreach ($period as $savedDate) {   
    //Check if the Item is Approved
    if ($event['Approved'] == "Approved"){
        switch($recurring){

Ok, think about what you're doing here. For each date in the DatePeriod, you're evaluating, basically, what the results of the initial query told you. Why do you need to check those more than once? You know the $event['Approved'] and $recurring values aren't going to change. Determine which case is going to be true beforehand. Then you can significantly shorten the loop body.
You're only processing those events that have been approved! Why not add that to the WHERE clause in your query????

SELECT * FROM calendar WHERE Approved = 'Approved' ORDER BY starttime;

That way, you don't have to check the value of $event['Approved'] to begin with. Also: break; is the same as writing break 1;. The latter just looks weird here. Anyway, consider writing separate functions for various event-types: renderNonRecurring, renderDailyEvent and (but there's a lot to be said about this case still) renderWeeklyEvent.
You can then write something as simple as:

foreach ($period as $savedDate)
{
    switch ($recurring)
    {
        case 'None':
            if ($gendate == $savedDate)
            {//DateTime instances can be compared like so, no format needed
                renderNonRecurringEvent($event);
            }
            break;
    }
}

Notice how I don't pass every individual key of the array to the render function, but instead just pass all of the event-related data. Doesn't that make sense to you?
Of course, looking at this function's tendency to echo, I take it your render* functions echo, too. Just have them return the output string and concatenate it to the $outString I mentioned in the beginning of my answer:

$outString .= renderEvent($event);

Now, for the big one:

Case 'Weekly':
    //Check margin between start and end date of recurring event
    if ($displaydate > $startdate and !$displaydate < $enddate){

Operator precedence... this condition is just terribly unreliable. and has a low precedence. Use &&. Always. Unless you know what you're doing.
Also think about what you're trying to check when you write

!$displaydate < $enddate

Are you saying

(!$displaydate) < $enddate
//if inverse boolean value of $displaydate < $enddate
//ie: if $displaydate truthy, then this would evalute to:
//    if (!true) < $enddate -> false < $enddate --> 0 < $enddate

Or are you trying to check for:

$displaydate >= $enddate //makes a lot more sense, no?

For some reason, you've created a function to get the diff in weeks. What is odd is that you insist on passing the date string to this function, when you've already constructed a DateTime instance for these dates. At least pass that to the function, because I'm prepared to take a punt that this datediffInWeeks function creates those same instances all over. But to be honest, I'd just not bother, and write this in-line, there's not a lot too it anyway. Here's the code you have:

        //Find the amount of weeks between two dates
         $weekRange = datediffInWeeks($recur['startdate'], $recur['enddate']);
         //Round down to the possible amount to display
         $weeks = ceil($weekRange / $recur['day']);

And this is what I'd write:

$weekRange = $recur['startdate']->diff($recur['enddate']);
$weeks = range(0, ceil($weekRange->d/7));//d property is number of days, as int
//to get range of number of weeks:
$weeks = range(
    (int)$recur['startdate']->format('W'),//start from current week
    $recur['startdate']->format('W') + ceil($weekRange->d/7)
);

Now once you have that, there is no point in looping over the array, is there? a simple in_array call, or even if (min($weeks) <= $gendate->format("W") && max($weeks) >= $gendate->format("W")) would do the trick.
The same logic applies to the days business. That way, you can do away with all those nested loops, because that's just an unholy, slow, messy and unmaintainable mess.

PDO: Reusing prepared statements:
Here's an example of how I'd query the data using PDO, re-using prepared statements:

//outside the loop, call prepare
$stmt = $pdo->prepare('SELECT * FROM recur WHERE title = :title AND name = :name');
//note no quotes, just :title and :name
$events = $pdo->query('SELECT * FROM calendar WHERE Allowed = "Allowed" ORDER BY starttime ASC');//order by <field> ASC/DESC
while ($event = $events->fetch(PDO::FETCH_ASSOC))
{//inside call execute as much as you want
    $stmt->execute(
        array(
            ':name'  => $event['name'],
            ':title' => $event['title']
        )
    );
    $recur = $stmt->fetch(PDO::FETCH_ASSOC);
}

General recommendations
Refactor this code ASAP. Learn about more modern MySQL extensions, PDO or mysqli_*. Both are more powerful than mysql_*, but mysqli_* is the most powerful of the lot. However, its API is messy (alowing both OO and procedural style programming), and has a lot more pitfalls owing to its complexity.
I haven't touched on this in my answer, but never assume all is going well. Check what functions return. They could return false, 0 or null, or they could throw an Exception, to let you know all is not well. Don't ignore those situations, deal with them.
Write a couple of one-liners down as guide lines, for example:

  • Prepare queries that use variables in the WHERE clause, by using prepared statements
  • If you have to scroll to read through a function, you're doing too much in one function. Split the logic over several functions.
  • DRY
  • Only SELECT what you need
  • Functions don't echo, they return. Think of them as books. They contain information, you read it, and can then relay that information to others. A book doesn't read itself out loud to other people. That's not its function.
  • errors happen. That's a fact. Check the return values of functions (false, 0, null or wrap them in a try-catch block). Check the manual, to see what each function returns in case something goes wrong.
  • Learn about prepared statements and injection. This implies changing the MySQL extension you use, this page helps you with that
  • Debugging implies seeing the bugs. Therefore E_STRICT | E_ALL + display_errors are a must.
share|improve this answer
    
Thank you so much for the hard work. I am new to php but I understand what you are saying and I will post my new code. Thanks –  Pieter de Vries Mar 31 at 18:03
    
@PieterdeVries: It's all in a day's work for bicycle repair man ;) Do let me know when and where you've posted your updated code, in case you want it reviewed –  Elias Van Ootegem Apr 1 at 7:24
    
Well I have some issues with figuring out some of the things. I am still perfecting it and just so you know the case clause is for repeated events. You may see the form at logicalwebhost.com/goats/calendar/calendar_entry.php BTW the reason I used mysql instead of opd or MySQLi is because the project was started in that by another user but I did know about the change in php. –  Pieter de Vries Apr 2 at 18:18
    
Here is my new code: codereview.stackexchange.com/questions/46133/… –  Pieter de Vries Apr 3 at 2:59
add comment

Here are a few quick tips:

  • Your indentation seems mostly consistent, but there are a few lines off by a space. Most editors have a format feature if you find it too tedious to maintain it manually.

  • Other whitespace is inconsistent as well. Compare these two lines:

    foreach ($period as $savedDate) {        // perfect
    

    and

    switch($recurring){                      // allergic to spaces?
    
  • Pick a naming convention and stick with it: $startdate versus $savedDate. I recommend camelCase.

  • break statements don't need the default 1 argument. Inside switch blocks especially it looks very strange and makes you stop and think when there's no reason.

  • Indent the break statements one level below their matching case statements, at the same level as the code before them. Some people keep the case lines at the same level as the switch since they go hand-in-hand to keep the lines from shifting so far to the right.

Bug

Be very careful when negating boolean expressions. if (!$recurring == "None") will never pass because no value equals the string "None" when it's negated. This should be if (!($recurring == "None")) to correct the operator precedence, but if ($recurring !== "None") is clearer.

Finally, the best way to see how it performs is to time it repeatedly and take the average.

share|improve this answer
    
White space use is largely a personal taste (as long as it is not on the level of for(int i=0,int j=3;i<2*j+1,j>=34;i+=2,j/=2*3+c) ) and I find both of the given examples acceptable. Although OP would benefit from being consistent. –  Emily L. Mar 31 at 9:15
    
The operator precedence bug is present all over.. most notably if ($displaydate > $startdate and !$displaydate < $enddate): using and is preventing this to be evaluated in the wackiest of ways, but it doesn't betray much awareness of operator precedence at all... –  Elias Van Ootegem Mar 31 at 11:36
add comment

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.