Am I headed in the right direction with PHP?

Question

I have been doing PHP for about three years now, with little direction other than the books I read and trial and error.

I am currently coding what I hope to be a pretty nice application and I have decided to throw myself into the fire and get torn apart (haha you think this is a nice application!!)

Over the last year I have tried to improve my coding by separating it better and moving towards an MCV type idea. At this point most of my work is procedural but I am starting to finally get into the OOP side and at least get my feet wet.

the code below is what I believe to be my controller I think? But really that isn't too important, I am more concerned about whether or not what I am trying to do in this script is being attacked in the right way.

This is for a lodge/resort application, we have ten different cabins and this page, cabins.php is responsible for pulling the various data needed for each cabin.

Please note that my comments are to explain my thoughts to this group, they are not proper comments in the true sense, simply to try and help explain what I think is going on!

If this is to broad or helpless, forgive me.

Here we go:

<?php
include("includes/configure.php");
include("includes/content_generation.php");



//Generate sub menus and collect info from database
if ($filename == 'cabins'){
$cabin_title = 'Neato Lodge Cabins';

    if(isset($_GET['cabin'])){
      $cabin = $_GET['cabin'];
        if(!isset($type)){
        header('Location: http://www.mysite.com/cabins.php?cabin='.$cabin.'&type=cabin_information');
        }
      $sub_menu_items['cabin_information']= 'Cabin Information';
      $sub_menu_items['reservation_info']= 'Reservation Info';
      $sub_menu_items['rates']= 'Rates';
      $sub_menu_items['cabin_pictures']= 'Cabin Pictures';




    if(isset($_GET['type'])){
        $type = $_GET['type'];

        switch ($type){

        case 'cabin_information':
        //I do a join here because I am storing cabin photos in their own table that shares cabin ID. This is my feeble attempt at trying to normalize my data, seemed like a good start. The image pulled is needed later in the view.
         $query = "SELECT cabin_name,cabin_description,bed_one,bed_two,bed_extra,image FROM cabin_content  INNER JOIN    cabin_images  ON cabin_content.cabin_id = cabin_images.cabin_id WHERE cabin_content.cabin_id = '$cabin'";
          $data = mysqli_query($dbc, $query);
          $row = mysqli_fetch_array($data);

          $main = '<p class ="first">'.$row['cabin_description'];
          $main .=  '<p class ="big">Sleeping Information</p>
          <p class ="bedrooms">Bedroom One: <span class = "bed">'.$row['bed_one'].'</span></p>
          <p class ="bedrooms">Bedroom Two: <span class = "bed">'.$row['bed_two'].'</span></p>
          <p class ="bedrooms">Additional Sleeping: <span class = "bed">'.$row['bed_extra'].'</span></p>
          <p>For occupany information, available dates and other reservation rules click the reservation info tab</p>';

          break;

            case 'reservation_info':

            $query = "SELECT cabin_name, peak_week, max_occupancy,image FROM cabin_content  INNER JOIN cabin_images  ON cabin_content.cabin_id = cabin_images.cabin_id WHERE cabin_content.cabin_id = '$cabin'";
              $data = mysqli_query($dbc, $query);
              $row = mysqli_fetch_array($data);

              $main = '<p class ="first">'.$row['cabin_name'].' is available to be rented from May through September, please read below for occupancy information and reservation rules and requirements. '.$row['cabin_name'].' has a maximum occupancy of '.$row['max_occupancy'].' adults for the posted rates, extra charges apply for additional adults or children.</p>
             <p class ="big">Seasonal Reservation Information</p>
             <p class ="bedrooms">Spring Season: <span class = "bed">'.$row['cabin_name'].' is available for rent with a five night minimum stay with a special discounted rate.</span></p>
              <p class ="bedrooms">Peak Season: <span class = "bed">'.$row['cabin_name'].' is available for rent with a six night minimum stay checking in on '.$row['peak_week'].' and checking out the following '.$row['peak_week'].'. If booking within thirty days of planned stay, cabin may be available with a five night minimum.</span></p>
              <p class ="bedrooms">Fall Season: <span class = "bed">'.$row['cabin_name'].' is available for rent with a three night minimum stay.</span></p>';


              break;

              case 'rates':

              $query = "SELECT cabin_name, spring_weekly, peak_weekly, fall_weekly, adult_weekly,spring_nightly, peak_nightly, fall_nightly, adult_nightly, youth_nightly, youth_weekly, max_occupancy,image FROM cabin_content  INNER JOIN cabin_images  ON cabin_content.cabin_id = cabin_images.cabin_id WHERE cabin_content.cabin_id = '$cabin'";
              $data = mysqli_query($dbc, $query);
              $row = mysqli_fetch_array($data);

              $main = '<p class ="first"> Weekly and nightly rates are for up to the cabin maximum occupancy of '.($row['max_occupancy']).'.</p>
               <p class ="big">Seasonal Rate Information</p>
            <table cellspacing="2" cellpadding="3" >
                  <tr>
                    <td class ="clm_head_lft">Season</td>
                    <td class ="clm_head">Weekly Rate</td>
                    <td class ="clm_head">Nightly Rate</td>
                    <td class ="clm_head">Additional Occupants</td>
                  </tr>
                  <tr>
                    <td class ="table_head">Spring Season (May 5th - June 15th:</td>
                    <td><span class = "rate"><del>'.$row['fall_weekly'].'</del> </span><span class = "special_rate">'.$row['spring_weekly'].'</span></td>
                    <td><span class = "rate">'.$row['spring_nightly'].'</span></td>
                    <td rowspan="3"><span class = "additional">Additional adults are '.$row['adult_weekly'].' per week and '.$row['adult_nightly'].' per night, youth under 18 are '.$row['youth_weekly'].' per week and '.$row['youth_nightly'].' per night. </span></td>
                  </tr>
                  <tr>
                    <td class ="table_head">Peak Season:</td>
                    <td><span class = "rate">'.$row['peak_weekly'].'</span></td>
                    <td><span class = "rate">'.$row['peak_nightly'].'</span></td>
                    <td></td>
                  </tr>
                  <tr>
                    <td class ="table_head">Fall Season:</td>
                    <td><span class = "rate">'.$row['fall_weekly'].'</span></td>
                    <td><span class = "rate">'.$row['fall_nightly'].'</span></td>
                    <td></td>
                  </tr>

                </table>';

                 break; 


        }
    }

 }



}

include("layout_includes/header.php");
include("layout_includes/cabins_layout.php");
include("layout_includes/footer.php");
?>

Answer 1

This is not a Controller. The purpose of MVC is separation of concerns, more specifically the separation of domain logic from the user interface.

Let's see where it fails:

$main .=  '<p class ="big">Sleeping Information</p>
           <p class ="bedrooms">Bedroom One: <span class = "bed">'.$row['bed_one'].'</span></p>
           <p class ="bedrooms">Bedroom Two: <span class = "bed">'.$row['bed_two'].'</span></p>
           <p class ="bedrooms">Additional Sleeping: <span class = "bed">'.$row['bed_extra'].'</span></p>
           <p>For occupany information, available dates and other reservation rules click the reservation info tab</p>';

Scenarios:

1. You need to add another css class to any of your paragraphs
2. You need to change a css class to any of your paragraphs
3. You need to convert a paragraph to anything else
4. You need to change the text of a paragraph
5. blah blah blah (there are a lot of other likely scenarios, but I think the first 4 are enough to illustrate my point)

Points 1 to 3 are user interface specific concerns. In an MVC approach they belong to the View and not in the Controller.

Point 3 can be a user interface concern, if the texts are static, but can also be viewed as a data concern. The same way you get data from the database, you could collect all these static texts in a configuration file and only have to look at one file to change them across every View in your application. That could be considered a Model approach. Having said that, this:

 $query = "SELECT cabin_name,cabin_description,bed_one,bed_two,bed_extra,image FROM cabin_content  INNER JOIN    cabin_images  ON cabin_content.cabin_id = cabin_images.cabin_id WHERE cabin_content.cabin_id = '$cabin'";
$data = mysqli_query($dbc, $query);
$row = mysqli_fetch_array($data);

Doesn't really belong to the Controller as well. The easiest approach would be to have all your database specific functionality in functions in a separate file:

// model file, lets call it "cabinModel.php"

function getCabinInformation($cabinID) {
    $query = "SELECT cabin_name,cabin_description,bed_one,bed_two,bed_extra,image FROM cabin_content  INNER JOIN    cabin_images  ON cabin_content.cabin_id = cabin_images.cabin_id WHERE cabin_content.cabin_id = '$cabinID'";
    return mysqli_query($dbc, $query);        
}

function getAllCabins() {
    ... 
}

function deleteCabin($cabinID) {
    ... 
}


// controller

include "cabinModel.php"

$row = getCabinInformation($cabin);

This way a function to get cabin information is available to all your controllers, you don't have to rewrite it every time you need it. Don't repeat yourself.

If there's any HTML / CSS or any other presentation logic in your Controller, and of course any persistent data logic, you are doing it wrong (in MVC terms). But MVC for small sites may be an overkill. It's a correct approach conceptually, but you will have to decide for yourself if it's the right one for your application. But if you decide it is, you should follow it as is.

A very easy approach to separate presentation from logic would be to use a template engine. There are quite a few of them out there, and there isn't one that's better than the others. Using one is more important than which one.

And of course it wouldn't hurt if you didn't try to reinvent the wheel and started using an MVC framework. Or if that feels too much, a micro framework.

There's is an often quoted article by Rasmus Lerdorf that some people perceive as advocating against template engines and frameworks. It's not, the only point of the article is that you don't have to use them. In the article there's a very nice and tidy approach on how to get an MVC kind of structure out of the box, without the added complexity of any third library. If you really don't want to use any third library, you should copy Rasmus' style.