PHP style critique

[bejitto101]

I think my code is horrible. Granted it works, but everything is a jumbled mess, and cluttered, everything shoved into one page. I was wondering if I could get some pointers on some ways to clean my code up.

Here's an example of one of my pages:

PHP Code:

<?php
    $id = $_GET['id'];

    if($id==""){
        header("Location: restaurants.php");
    }
    
    include 'include/session.php';
    include 'includes/prep3.php';
    include 'includes/header.php';
    include 'includes/functions.php';
    include 'includes/happenings.class.php';
    include 'includes/businesses.class.php';
    
    $query = "SELECT page FROM businesses b JOIN business_categories bc on bc.id = b.category WHERE b.id = ?";
    $stmt = $mysqli->stmt_init();
    $stmt->prepare($query) or die ("Could not prepare statement:" . $stmt->error);
    $stmt->bind_param('i', $id) or die ("Could not bind parameters statement:" . $stmt->error);
    $stmt->bind_result($page_name) or die ("Could not bind results:" . $stmt->error);
    $stmt->execute() or die ("Could not execute statement:" . $stmt->error);
    $stmt->fetch();
            
    if($_SERVER['SCRIPT_NAME'] != ("/".$page_name))
        header("Location: $page_name?id=$id");
    
    $stmt->reset();
    
    //Lets pull the data    
    $stmt = $mysqli->stmt_init();
    $stmt->prepare("SELECT id, name, address, city, zip, lat, lng, phone, area, website, category, tags, description, hours, picture FROM businesses WHERE id=?") or die ("Could not prepare statement:" . $stmt->error);
    $stmt->bind_param('i', $id) or die ("Could not bind parameters statement:" . $stmt->error);
    $stmt->bind_result($id, $name, $address, $city, $zip, $lat, $lng, $phone, $area, $website, $category, $tags, $description, $hours, $picture) or die ("Could not bind results:" . $stmt->error);
    $stmt->execute() or die ("Could not execute statement:" . $stmt->error);
    $stmt->fetch();
    
    $stmt->reset();
    
    $stmt->prepare("SELECT * FROM restaurants WHERE id=?") or die ("Could not prepare statement:" . $stmt->error);
    $stmt->bind_param('i', $id) or die ("Could not bind parameters statement:" . $stmt->error);
    $stmt->bind_result($id, $cuisine_1, $cuisine_2, $cuisine_3, $nightlife_1, $nightlife_2, $nightlife_3, $happy_hour, $late_happy_hour) or die ("Could not bind results:" . $stmt->error);
    $stmt->execute() or die ("Could not execute statement:" . $stmt->error);
    $stmt->fetch();
    
    $stmt->close();

    
    //Some header fun, sets header tags
    $header = new HeaderClass();
    $header->title = htmlspecialchars($name). ' - Bellevue WA &amp; Eastside | Bellevue.com';
    $header->js = 'js/restaurant.js';
    $header->js ='http://maps.google.com/maps?file=api&amp;v=2&amp;key=AB';

    $header->css = 'css/restaurant.css';
    //$header->meta = Array('http-equiv'=>'Content-Type', 'content'=>'text/html; charset=utf-8');
    $header->meta = Array('name'=>'description', 'content'=>'some text here for search engines');
    unset($header);
    
    if(!empty($cuisine_1))
        $cuisines[] = $cuisine_1;
    if(!empty($cuisine_2))
        $cuisines[] = $cuisine_2;
    if(!empty($cuisine_3))
        $cuisines[] = $cuisine_3;
    if(!empty($nightlife_1))
        $nightlife[] = $nightlife_1;
    if(!empty($nightlife_2))
        $nightlife[] = $nightlife_2;
    if(!empty($nightlife_3))
        $nightlife[] = $nightlife_3;
    
    $happenings = new Happenings();
    $businesses = new Businesses();
    
    $name = htmlspecialchars($name);
    $category = htmlspecialchars($category);
?>


<div id="leftCol">
    <div id="rest-info">
        <div id="rest-img">
            <?
                if($picture=="")
                    $picture="images/bellevue300x250.gif";
                else $picture = "uploads/images/business/".$picture;
            ?>
            <img src="<?=$picture?>" alt="Restaurant"/>
        </div>
        <div id="rest-text">
            <h1><?=$name?></h1>
            <?
                $topinfo = "";
                if(is_array($cuisines))
                    $topinfo .= "<p>".implode(", ",$cuisines)."</p>";

                $rating = round($rating);
                $stars="";
                for($i=0; $i<$rating; $i++){
                    $stars .= "<img src=\"images/star.gif\" alt=\"full star\" />";
                }
                for($i=0; $i<(5-$rating); $i++){
                    $stars .= "<img src=\"images/star_empty.gif\" alt=\"empty star\" />";
                }

                $topinfo .= "<p>$stars (".(int)$row['votes'].") Votes | <a href='#'>rate &amp; share your review</a></p><br />";

                if($website != "")
                    $topinfo .= "<p><a target='_blank' href='$website'>".shorten_url($website)."</a></p>";
                if($phone != "")
                    $topinfo .= "<p><strong>Phone:</strong> $phone</p><br />";
                if(is_array($nightlife))
                    $topinfo .= "<p><strong>Nightlife:</strong> ".implode(", ",$nightlife)."</p>";
                if($happy_hour != "")
                    $topinfo .= "<p><strong>Happy Hour:</strong> $happy_hour</p>";
                if($late_happy_hour != "")
                    $topinfo .= "<p><strong>Late Happy Hour:</strong> $late_happy_hour</p><br />";
                if($tags != "")
                    $topinfo .= "<p><strong>Tags:</strong> $tags</p>";
                    
                echo $topinfo;
            ?>
        </div>
        <br class="clear" />
    </div>

    <div id="map-wrapper">
        <?
            $full_address = $address.", ".$city." ".$zip;
        ?>
        <p><?=$full_address?> | <a target="_blank" href="http://maps.google.com/maps?saddr=&amp;daddr=<?=$full_address?>&amp;hl=en">Directions</a></p>
        <div id="map" style="height:370px; width:460px;"></div>
        <?
            $businesses->whats_nearby();
        ?>
        <br class="clear" />
    </div>
    <div id="rest-descrip">
        <div class="menu-items"> <a id="profile" class="restcontrols selected">Profile</a><a class="restcontrols" id="reviews">Reviews</a><a class="restcontrols" id="happenings">Happenings</a><br class="clear" /></div>

        <div class="forms">
            <div id="profile_control" class="x">
                <? if($description!=""){ ?>
                    <h2>About <?=$name?></h2>
                    <p><?=$description?></p><br />
                <? } ?>
                <div class="col-wrapper">
                    <div class="left-col">
                    <?
                        if($hours != "")
                            $botinfo .= "<p><strong>Hours:</strong> $hours</p>";
                        if($happy_hour != "")
                            $botinfo .= "<p><strong>Happy Hour:</strong> $happy_hour</p>";
                        if($late_happy_hour != "")
                            $botinfo .= "<p><strong>Late Happy Hour:</strong> $late_happy_hour</p><br />";
                        echo $botinfo;
                    ?>
                    </div>
                    <div class="right-col">
                        <h3>Happenings at <?=$name?></h3>
                        <p>Got an event or happening? <a href="happenings_entry.php">Post your event here!</a></p>
                        <br />
                        <?
                            $happenings->list_id_condensed($id);
                        ?>
                    </div>
                    <br class="clear" />
                </div>
                <div id="rest-reviews">
                    <h2>User reviews for <?=$name?></h2>
                    <p><a href="#">Log in and share your review</a></p>
                </div>
            </div>
            <div id="reviews_control"></div>
            <div id="happenings_control">
                <h2>Happenings at <?=$name?></h2>
                <p>Got an event or happening? <a href="happenings_entry.php">Post your event here!</a></p>
                <br />
                <?
                    $happenings->list_id($id);
                ?>
            </div>
        </div>
    </div>
</div>
<div id="rightCol">
    <?php
        include("modules/search.php");
        include("modules/rest_featured.php");
        include("modules/happenings.php");
    ?>
</div>

<?php
    include("includes/footer.php");
?>

It uses some classes, includes some files, but it just looks messy and disorganized. I don't know what I can do to fix it. I have several pages like this, basically pulling information out of a database and displaying it.

My input forms are even more horrid and I can include one of those too if anyone is willing to help me pick up some better coding styles.

Any help will be greatly appreciated!

[Shorts]

Trust me, I've seen way worse... One of my co-workers doesn't quite get the idea of Classes so he ends up making 3-4 different ones for the pages he's making and uses them sort of like a spaghetti\procedural hybrid.

As for this it's more of a personal choice. Unless you work somewhere that has unified code (I will finally have that).

Personally I go with more of a template style of coding. Looking at this page I notice you have the dynamic header and footer going on. I go the other way, each inside page is dynamically added to a main template.

Looking at your code you are consistent which is another major thing. Noticed you use the if else without brackets but again you're consistent.

As for me (coming from perl to PHP) everything that it's optional to have brackets has brackets. Again, that's personal,

so my code looks like:

   if($something==true) { print 'cool'; }
   if($bacon=='is tasty') {
      print 'You know it!';
      exit;
   }

(All 1 line if\elses are on the same line, otherwise they are broken up).

Two last tidbits. Here all classes are first letter capital $Class = new Class(); and all variables\arrays are first case lower $some_variable (yeah a fan of the underscore).

The other tidbit, use a colored\formatted text editor like Notepad++ if you don't already do. That's my weapon at home, work it's Dreamweaver (Code view only) due to the setups.

Wow, longer than I thought that'd be...

[Mindzai]

A couple of things which will have a big impact. Firstly try to get away from procedural programming for all but the smallest scripts (and some would argue even for those) and learn object oriented programming. It takes a while to get to grips with, but once you get it you will wonder how you ever coded any other way. Another tip is to separate your logic from your presentation - seeing lots of html mised in with PHP all the way through a script is never a good sign. It will make your life easier when it comes to debugging too. I personally tend to go one step further than that and use the MVC paradigm in larger projects in order to separate data management, business logic and presentation, but i'd worry about that after learning OOP.

[bejitto101]

Shorts: Could you elaborate on how you work everything through a template? Thanks for all your other suggestions. As for my editor, I use Dreamweaver basically just because of the ability of uploading files automatically when I save them. I would gladly try any other editor though because Dreamweaver is a resource hog and all I use it for is a text editor and a ftp solution. Any suggestions?

Mindzai: Well I understand OOP, I can even create classes. I have a couple small classes that I utilize for various functions. I know it would help me in situations like this because I have several pages like this.

My biggest issue with OOP, and I've been struggling with this for a while, is how to apply it in situations like this. What type of class(es) would I make here just for simple presentation of data? I guess I just have trouble splitting my data into classes where it would be simpler than some procedural code. Can you suggest any practices or give me some examples that would help me out here?

As for mixing logic and presentation, just do all my logic at the top of the file basically? And then when I want to display info, just use something like "<?=$some_info?>"? That wouldn't be hard to fix, and would make it look cleaner, and I've actually thought about doing that a couple times.

Thank you both for you suggestions!

[NogDog]

When I see this...

PHP Code:

$query = "SELECT page FROM businesses b JOIN business_categories bc on bc.id = b.category WHERE b.id = ?";
    $stmt = $mysqli->stmt_init();
    $stmt->prepare($query) or die ("Could not prepare statement:" . $stmt->error);
    $stmt->bind_param('i', $id) or die ("Could not bind parameters statement:" . $stmt->error);
    $stmt->bind_result($page_name) or die ("Could not bind results:" . $stmt->error);
    $stmt->execute() or die ("Could not execute statement:" . $stmt->error);
    $stmt->fetch();
            
    if($_SERVER['SCRIPT_NAME'] != ("/".$page_name))
        header("Location: $page_name?id=$id");
    
    $stmt->reset();
    
    //Lets pull the data    
    $stmt = $mysqli->stmt_init();
    $stmt->prepare("SELECT id, name, address, city, zip, lat, lng, phone, area, website, category, tags, description, hours, picture FROM businesses WHERE id=?") or die ("Could not prepare statement:" . $stmt->error);
    $stmt->bind_param('i', $id) or die ("Could not bind parameters statement:" . $stmt->error);
    $stmt->bind_result($id, $name, $address, $city, $zip, $lat, $lng, $phone, $area, $website, $category, $tags, $description, $hours, $picture) or die ("Could not bind results:" . $stmt->error);
    $stmt->execute() or die ("Could not execute statement:" . $stmt->error);
    $stmt->fetch();
    
    $stmt->reset();
    
    $stmt->prepare("SELECT * FROM restaurants WHERE id=?") or die ("Could not prepare statement:" . $stmt->error);
    $stmt->bind_param('i', $id) or die ("Could not bind parameters statement:" . $stmt->error);
    $stmt->bind_result($id, $cuisine_1, $cuisine_2, $cuisine_3, $nightlife_1, $nightlife_2, $nightlife_3, $happy_hour, $late_happy_hour) or die ("Could not bind results:" . $stmt->error);
    $stmt->execute() or die ("Could not execute statement:" . $stmt->error);
    $stmt->fetch();
    
    $stmt->close();


...my first instinct is that this sort of functionality would be a candidate for turning into classes/objects. If this is the only place you do such queries, maybe not, but they look like things that might be used in more than one place in the application, which makes them candidates for modularization via OOP (or at least by functions). Then on this page you'd replace each group of query lines with one or two: instantiating the object then calling the needed method.

PS: As far as editors, I've been using NetBeans 6.5 lately and find it to be a pretty good combination of features, performance, and price (free). See this article for one thing that I did not find at all intuitive about it, though.

[bejitto101]

NogDog: So basically have a class thats comprised of queries? So basically each function is something like:

PHP Code:

function pull_business(){
    $stmt = $mysqli->stmt_init();
    $stmt->prepare("SELECT id, name, address, city, zip, lat, lng, phone, area, website, category, tags, description, hours, picture FROM businesses WHERE id=?") or die ("Could not prepare statement:" . $stmt->error);
    $stmt->bind_param('i', $id) or die ("Could not bind parameters statement:" . $stmt->error);
    $stmt->bind_result($id, $name, $address, $city, $zip, $lat, $lng, $phone, $area, $website, $category, $tags, $description, $hours, $picture) or die ("Could not bind results:" . $stmt->error);
    $stmt->execute() or die ("Could not execute statement:" . $stmt->error);
    $stmt->fetch();
}


I mean if it wasn't a prepared statement, I could return an object but in this case I can't return anything. I would love to add more classes to handle everything, I just don't know where to.

[NogDog]

The idea is to think in "objects". For instance, looking at that query in the preceding post, presumably you have some "thing" you are modeling in your application called a "business". So the OODesign approach would be to create a Business object. This object would store the data (class variables) for a given business as well as the things you might do with that data (class methods). One of those methods would likely be for populating the Business object data from the database. Its argument would be the ID number, which would then be used in a query to get the data:

PHP Code:

<?php
class Business
{
   private $db;
   private $data = array(
      'id' => null,
      'name' => null,
      'address' => null,
      'city' => null,
      'zip' => null,
      'lat' => null,
      'lng' => null,
      'phone' => null,
      'area' => null,
      'website' => null,
      'category' => null,
      'tags' => null,
      'description' => null,
      'hours' => null,
      'picture' => null
   );
   
   public function __construct(mysqli $db, $id = null)
   {
      if(!empty($id))
      {
         $id = (int)$id;
         $this->populate($id);
      }
   }

   public function __get($key)
   {
      if(array_key_exists($key, $this->data))
      {
         return $this->data[$key];
      }
      error_log("Invalid key '$key'");
      return null;
   }
   
   public function __set($key, $value)
   {
      if(array_key_exists($key, $this->data))
      {
         $this->data[$key] = $value;
         return true;
      }
      return false;
   }
   
   public function populate($id)
   {
      $stmt = $db->prepare("SELECT * FROM businesses WHERE id=?");
      $stmt->bind_param('i', $id);
      $result = $stmt->execute();
      if($result)
      {
         if($result->num_rows == 1)
         {
            $this->data = $result->fetch_assoc();
         }
         else
         {
            error_log($result->num_rows . " returned for id '$id'");
            return false;
         }
      }
      else
      {
         error_log($stmt->error());
         return false;
      }
      return true;
   }
}

Then in your main page where you need to reference a particular business:

PHP Code:

$db = new mysqli('localhost','root','######','database_name');
$bus = new Business($db, $id);
// output the name:
echo $bus->name; // uses the __get() method


Presumably this Business object would also have insert, update, and delete methods. Then it could be used in any of your code where you need to access/manipulate a business in your application, giving you one central point to deal with all the code that performs such operations.

To take it a step further, you could have an abstract class that contains all the basic functionality, and then have each specific class (such as Business) extend that abstract class, only then needing to add code specific for that object type (such as the keys for the $data array).

[ishudan]

thanks for sharing

[bejitto101]

Thanks NogDog for the advice.

Unfortunately, I don't believe you can use prepared statements to get a result set with mysqli, a big drawback in my opinion. I really like prepared statements, it makes things a little simpler. The only thing you can do with a prepared statements is bind the parameters. I guess I would just have to bind each variable to $data individually, or is there a better way to do this?

[NogDog]

Originally Posted by bejitto101

Thanks NogDog for the advice.

Unfortunately, I don't believe you can use prepared statements to get a result set with mysqli, a big drawback in my opinion. I really like prepared statements, it makes things a little simpler. The only thing you can do with a prepared statements is bind the parameters. I guess I would just have to bind each variable to $data individually, or is there a better way to do this?

PHP Code:

   public function populate($id)
   {
      $sql = sprintf(
         "SELECT `%s` FROM `example_table` WHERE ID = ?",
         implode("`, `", array_keys($this->data))
      );
      $stmt = $this->db->prepare($sql);
      $stmt->bind_param('i', $id);
      $stmt->execute() or die('exec');
      $stmt->store_result();
      if($stmt->num_rows == 1)
      {
         $params = array();
         foreach($this->data as $key => $val)
         {
            $params[] = &$this->data[$key];
         }
         call_user_func_array(array($stmt, 'bind_result'), $params);
         $stmt->fetch();
         $return = true;
      }
      else
      {
         user_error("No rows returned for id '$id'");
         $return = false;
      }
      return $return;
   }