Coding Techniques

PHP
1

I read through the pinned forums at the top, but just had to ask. I am very OCD about things, and wanted to ask if anything was wrong with this bit of code, technique wise.

I love to code and can usually make something work, but it turns out to be horrible looking -code wise- and if anything, that is what makes it so hard for me to program.

I added a snippet and would like to ask, “what are some tips that could make this more visually appealing?”


 
<?php
 
function users() {
 
$page = <<<END
<h3>Users</h3>
<p>Click a username to edit the account.</p>
<table id="info">\

END;
 
  $query = doquery("SELECT id, username, email, authlevel " .
                   "FROM {{table}} " .
                   "ORDER BY id", "users");
 
  $count = 1;
 
  while ($row = mysql_fetch_array($query)) {
    if ($count == 1) {
      $page .= "<tr class=\\"alt\\"><td>".$row["username"]."</td><td>".$row["email"]."</td><td class=\\"id\\">".$row["authlevel"]."</td></tr>\
";
      $count = 2;
    } else {
      $page .= "<tr><td>".$row["username"]."</td><td>".$row["email"]."</td><td class=\\"id\\">".$row["authlevel"]."</td></tr>\
";
      $count = 1;
    }
  }
 
  if (mysql_num_rows($query) == 0) {
    $page .= "<tr class=\\"alt\\"><td>No users found.</td></tr>\
";
  }
 
  $page .= "</table>";
 
  admindisplay($page, "Users");
 
}
 
?>
2

I’ve personally never liked the use of curly braces being inline like this } else { - We had a poll here recently to see who formats their curly braces in what style and despite being more confusing IMO that style came out the winner for some odd reason.

Also $row[“username”] only needs to be $row[‘username’]. Note the single quotes. Unless you’re doing something like $row[$username] you don’t need the double quotes for an array (and as you can see you don’t need them for a variable either) so you’re just forcing php to check it and waste resources for nothing. " are for use when you’re using a variable. ’ are for use when there is no variable.

Easy way to remember which one to use: ’ : normal (normal text) " not normal (not normal text because its a variable). Basically count the quote marks. 1 is normal 2 is not normal.

Also in your SQL you don’t need to use () or {} around the tablename.

You also did this:


$page .= "<tr class=\\"alt\\"><td>".$row["username"]."</td><td>".$row["email"]."</td><td class=\\"id\\">".$row["authlevel"]."</td></tr>\
";

All you needed to do was this:


$page .= "<tr class=\\"alt\\"><td>$row[username]</td><td>$row[email]</td><td class=\\"id\\">$row[authlevel]</td></tr>\
";

Hope that helps,

TF

3

When I was mixing my SQL with the rest of my scripts I got into the habit of formatting my sql statements like this.


  $query = doquery("SELECT id
         , username
         , email
         , authlevel
        FROM {{table}}
        ORDER BY id"
        , "users");

The reasons are:
1 it jumps off the page that this is sql
2 preceding commas on the fields makes it VERY EASY to spot when you miss a comma, and reduces the chance of error if you later decide to add a new field
3 As you did anyway, try and use caps to write the SQL joins, selects etc

You may disagree, and so will others but there is my POV.

My biggest bugbear with your code though (apart from the quoting as dealt with by tangoforce) is that you are writing 2 lines of html which only differ by one piece

class=“alt”

In this case its only one redundant line, but it just makes my teeth jar because you are making extra work for yourself if you ever change the table to say, add an extra column, there is a good chance you may miss the 2nd case.

The principal is “isolate what changes”.


  $first_row = true; // name it what it is for clarity
  $class = '' ;  // set default value 
 
  while ($row = mysql_fetch_array($query)) {
    if ($first_row === true) { // be specific about the test
      $class = 'class="alt" ';
      $first_row = false;
    }
   $page .=  '<tr ' . $class . '><td>' . $row['username'].'</td><td>'. $row['email'].'</td>
   <td class="id">'.$row['authlevel'].'</td></tr>' . PHP_EOL;
  }
// ** all untested

You have taken the happy path here, what happens if there is no data returned from the database?

As I say, this very much how I have evolved - and others may disagree and this is fine - do as much as you can to make sure that when you come back to this function in say 6 months time, you make is as easy to read as possible, and as difficult as possible to make a stupid error.

Overall though, mixing PHP, HTML, and SQL all in one function is something you should try and gradually move away from.

Brave question though.

4

While I agree with Cups re: SQL formatting, I typically do that during the development of the query. I then just shorten it in the codebase itself. Odd, I know, but that’s what I like to do.

Here’s how I would code your method

[LIST=1]
[]I only execute the loop if there are rows returned.
[]I used the modulus function to determine the row being displayed.
[*]I put each table cell on it’s own line. Yes, it’s a slight performance hit, but maintenance wise, it’s easier and faster to go back and maintain later.
[/LIST]I personally would have shifted the mysql calls over to mysqli, but since I didn’t know which version of php you were developing, I didn’t do that…


 <?php 
 function users() { 
    $page = "<h3>Users</h3>\
" .
            "<p>Click a username to edit the account.</p>\
" .
            "<table id=\\"info\\">";
    $query = doquery("SELECT id, username, email, authlevel " .
                       "FROM {{table}} " .
                      "ORDER BY id", "users");   
                      
    if (mysql_num_rows($query) == 0) {    
        $page .= "<tr class=\\"alt\\"><td>No users found.</td></tr>\
";  
    } else {
        $count = 1;   
        while ($row = mysql_fetch_array($query)) {
            $page .= "<tr". ($count % 2 ? "" : " class=\\"alt\\"") . ">" .
                        "<td>$row['username']</td>" .
                        "<td>$row['email']</td>" .
                        "<td class=\\"id\\">$row['authlevel']</td>" .
                     "</tr>";
            $count++;    
        }   
    }
    $page .= "</table>";   
    admindisplay($page, "Users"); 
} 
?>
5

The biggest problem I have with your code is that you are combining your business logic with your presentation layer. Much nicer is to use a simple templating system and pass the required information to the template to separate the model from the view. This is how I would like to see this written:

<?php
 
function users()
{
    $sql = "SELECT id, username, email, authlevel
            FROM {{table}} 
            ORDER BY id
    ";
    
    $query = doquery($sql, "users");
 
    $users = array();
    while ($row = mysql_fetch_array($query)) {
        $users[] = $row;
    }

    $template = new Template('Users');
    $template->addVariable('users', $users);
    $template->render();
}
 
?>

“Users” Template

<h3>Users</h3>
<p>Click a username to edit the account.</p>

<table id="info">
<?php

if (empty($users)) { ?>

    <tr class="alt"><td>No users found.</td></tr>
    
<?php
} else {

    foreach($users as $key => $user);
        $trClass = ($key % 0 == 1) ? ' class="alt"' : '';
        ?>
        
        <tr<?php echo $trClass; ?>>
            <td><?php echo $user["username"]; ?></td>
            <td><?php echo $user["email"]; ?></td>
            <td class="id"><?php echo $user["authlevel"]; ?></td>
        </tr>
        
        <?php
    }
}
?>
</table>
6

Thank you for all the kind replies.

@Tangoforce:
“Also in your SQL you don’t need to use () or {} around the tablename.”

The reason the tablename is {{table}} is it is sent to a DoQuery which appends a prefix to the table name at the end of the SQL, users.

I could have placed ws_users in it.

Also, thank you for trying to explain the use of ’ and ". I am still confused at which to use and when. There must be a million others just like me that just put whatever works because I see it all over the internet. I will try and look more into it.

@aamonkey

I have been trying to use templates. That is where the $page is sent to. I love how clean your code is, but it reminds me of a video on youtube trying to show me how to use OOP PHP, and I failed miserably in trying to understand it.

The first part is nice and clean, just how I want my code to look, but the second reminds me of what troubles me most with my code - Jumping in and out of PHP. Not sure there is any other way though.

I never can look at the source of my webpage and it line up indented correctly, which at times helps me debug my programming.

@Dave

I am using PHP 5.2.17 to answer your question.

I had tried using mod on the alternating lines like you did, but I always ended up using two lines.

This place is so much like school. I actually liked having a teacher to look at my things and say, “You could do this or that and make it better.” That is what this site has been doing… Helping me.

I could peruse the internet all day and not find the type of quality ideas I received from the replies.

Thanks and I hope I didn’t miss anything.

7

It’s simple.

If you are using text ‘like this which you can read’ you use a single quote.

“If you are using text with a $Variable then you use double quotes”.

For an $array[‘key’] the key is text so you use single quotes UNLESS the key is a variable.

Bottom line: ALWAYS use single quotes UNLESS there is a $Variable which you want PHP to replace.

8

You don’t have to use OOP to apply the same concept - You could create a function that does the same thing - the important thing is not to build the html in your model layer - you pass the variables that are needed to the template, not html.

Nothing says you have to jump in and out if you don’t like doing that - that’s just the way I prefer doing it.

Consider this:

<?php
 
function users()
{
    $sql = "SELECT id, username, email, authlevel
            FROM {{table}} 
            ORDER BY id
    ";
    
    $query = doquery($sql, "users");
 
    $users = array();
    while ($row = mysql_fetch_array($query)) {
        $users[] = $row;
    }

    renderTemplate('Users', array('users' => $users));
}
 
?>

Users template:

<h3>Users</h3>
<p>Click a username to edit the account.</p>

<table id="info">
<?php

if (empty($users)) {

    echo '<tr class="alt"><td>No users found.</td></tr>'."\
";
    
} else {

    foreach($users as $key => $user) {
        $trClass = ($key % 2 == 1) ? ' class="alt"' : '';
        echo '<tr'.$trClass.'><td>'.$user["username"].'</td><td>'.$user["email"].'</td><td class="id">'.$user["authlevel"].'</td></tr>';
    }
}
?>
</table>
9

@aamonkey

I just have to say, that last way of doing it, is exactly how I code… I think that just looks beautiful =D

10

I think its horrible lol. In my view a template should contain as little php as possible so I literally use TOKENS in my html template and then do all the looping, html generation etc in my php and then use str_replace to replace the appropriate token in the html.

11

A template should contain anything it needs to contain to completely control what is rendered to the browser. Doing all your html generation in the model layer defeats the purpose of separating the 2 in the first place. There is no ‘rule’ that says a template can’t contain php, in fact php throughout it’s core and history IS a templating language :wink:

12

I see what you’re saying but I’ve always thought that a template should remain that and that only -just a template. EG if a user designs their own template to install into your system but they don’t do PHP then they’re screwed. Thats why I completely seperated everything.

I even use templates for going into templates (EG menu templates to be merged into the main template etc). I’ve pretty much got it nailed to perfection for my own uses…

13

I’ve heard that argument before, but IMO it’s bogus - any halfway intelligent designer can understand basic if/then structures and loops. That’s why you keep all the heavy lifting stuff (db connecting, etc) out of the templates and in the model layer where it belongs.

Look at smarty - an entire templating language designed to be “more understandable” to non-coders. They invented another syntax so that non-techy people can plug in their variables and do their if/then statements and loops in the template layer without touching php…but it’s still another syntax they have to learn, it still employs display logic, and it’s no easier or harder than using php for it (again, IMO).

The problem with assuming designers are dumb and can’t be trusted with basic display logic code is that whenever you need a simple design change you have to get a coder involved and mess around in the guts of the application. That shouldn’t happen.

That’s my 2 cents anyway :stuck_out_tongue:

14

Eew, please don’t mention smarty near me again lol. I looked at it once and decided it just wasn’t right. PHP is in its own right a templating engine and if you do it right (which of course is my way:D) then you should never need something like smarty in the first place :wink:

I respect smarty for one thing though - over complicating the website development process :lol:

15

I was going to try and template it like you did, but I couldn’t figure out how to put a template into another template. Scratches his head


<?php
$template = <<<END
<head>
<title>{{title}}</title>
<meta http-equiv="Content-Type" content="text/html; charset=ISO-8859-1">
<link rel="stylesheet" type="text/css" href="admin.css">
</head>
<body>
  <div id="wrapper">
    <div id="nav">
      <div>
        <h4>Functions:</h4>
        <ul>
          <li><a href="admin.php?do=logout">Log out</a></li>
        </ul>
      </div>
    </div>
    <div id="content">
      {{content}}
    </div>
  </div>
</body>
</html>
END;
?>

I couldn’t seem to understand the concept of putting another template (below) in the {{content}} spot of the above template, it being a template in itself.


[COLOR=#000000][FONT=Courier New][/FONT][/COLOR] 
[COLOR=#000000][FONT=Courier New]<h3>Users</h3>
<p>Click a username to edit the account.</p>

<table id="info">
[/FONT][FONT=Courier New][COLOR=#0000bb]<?php

[/COLOR][COLOR=#007700]if (empty([/COLOR][COLOR=#0000bb]$users[/COLOR][/FONT][FONT=Courier New][COLOR=#007700])) {

    echo [/COLOR][COLOR=#dd0000]'<tr class="alt"><td>No users found.</td></tr>'[/COLOR][COLOR=#007700].[/COLOR][COLOR=#dd0000]"\
"[/COLOR][/FONT][FONT=Courier New][COLOR=#007700];
    
} else {

    foreach([/COLOR][COLOR=#0000bb]$users [/COLOR][COLOR=#007700]as [/COLOR][COLOR=#0000bb]$key [/COLOR][COLOR=#007700]=> [/COLOR][COLOR=#0000bb]$user[/COLOR][/FONT][FONT=Courier New][COLOR=#007700]) {
        [/COLOR][COLOR=#0000bb]$trClass [/COLOR][COLOR=#007700]= ([/COLOR][COLOR=#0000bb]$key [/COLOR][COLOR=#007700]% [/COLOR][COLOR=#0000bb]2 [/COLOR][COLOR=#007700]== [/COLOR][COLOR=#0000bb]1[/COLOR][COLOR=#007700]) ? [/COLOR][COLOR=#dd0000]' class="alt"' [/COLOR][COLOR=#007700]: [/COLOR][COLOR=#dd0000]''[/COLOR][/FONT][FONT=Courier New][COLOR=#007700];
        echo [/COLOR][COLOR=#dd0000]'<tr'[/COLOR][COLOR=#007700].[/COLOR][COLOR=#0000bb]$trClass[/COLOR][COLOR=#007700].[/COLOR][COLOR=#dd0000]'><td>'[/COLOR][COLOR=#007700].[/COLOR][COLOR=#0000bb]$user[/COLOR][COLOR=#007700][[/COLOR][COLOR=#dd0000]"username"[/COLOR][COLOR=#007700]].[/COLOR][COLOR=#dd0000]'</td><td>'[/COLOR][COLOR=#007700].[/COLOR][COLOR=#0000bb]$user[/COLOR][COLOR=#007700][[/COLOR][COLOR=#dd0000]"email"[/COLOR][COLOR=#007700]].[/COLOR][COLOR=#dd0000]'</td><td class="id">'[/COLOR][COLOR=#007700].[/COLOR][COLOR=#0000bb]$user[/COLOR][COLOR=#007700][[/COLOR][COLOR=#dd0000]"authlevel"[/COLOR][COLOR=#007700]].[/COLOR][COLOR=#dd0000]'</td></tr>'[/COLOR][/FONT][COLOR=#007700][FONT=Courier New];
    }
}
[/FONT][/COLOR][FONT=Courier New][COLOR=#0000bb]?>
[/COLOR]</table>[/FONT][/COLOR][FONT=Courier New] [/FONT]
16

It’s remarkably easy really. You use str_replace :wink:

In your second template with php you’d need to use output buffering to capture the output instead of sending it to the browser. Look up ob_start on php.net and ob_get_clean which will let you save the output buffer to a variable for use in str_replace.