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>;}
I have a huge function filled with nested blocks
php;beginner
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)//andif ($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 toughNow, 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 date1That 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 INTWhy, 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 rangeforeach ($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 < $enddateAre 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 < $enddateOr 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/DESCwhile ($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 recommendationsRefactor 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 statementsIf you have to scroll to read through a function, you're doing too much in one function. Split the logic over several functions.DRYOnly SELECT what you needFunctions 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 thatDebugging implies seeing the bugs. Therefore E_STRICT | E_ALL + display_errors are a must.