Common Style Mistakes, Part 1
by John Coggeshall05/29/2003
Introduction
With some PHP basics under our belt, it's time to introduce one of the most important things when writing web applications: security. I don't mean just protecting credit cards or other personal information. Rather, I am talking about writing secure, solid code from the very first line to the very last. With this topic, which I call "PHP Paranoia", I intend to teach you not only the function calls necessary to accomplish a task, but also the thought processes and practices to do so safely.
I recommend that you look at the columns in this series not as simple examples of using whatever functionality has been chosen but, rather, as general examples that can be applied to a number of different situations. I'll start my discussion by pointing out a few common stylistic mistakes made by PHP newcomers.
Errors in Style
Although they can't be considered fatal errors, bad stylistic habits can lead to a number of problems in a large-scale development project. Often these practices don't become apparent until you upgrade to a newer version of PHP. But they can also add hours to your development time.
To help you clearly see the differences between what is considered bad and good style let's take a look at some contrasting examples.
Tip 1: Avoid optional configuration directives
Consider the following code:
<?
$var = 0;
function myfunc($myvar) {
$myvar = 1;
}
echo "The value of the variable is: $var<br>";
myfunc(&$var);
echo "The value of the variable is: $var<br>";
?>Although this code will work, it relies on PHP configuration directives
to function properly. For instance, what would happen if the PHP
configuration directive short_tags were disabled? Since this
code does not use <?php to begin a code block, the entire
script would be output directly to the browser! Although not a serious
error for this script, for a script containing more sensitive data it
would be a real security risk.
There is also a problem here with the way our variable
$var is being passed to the function. Although it's not a
problem to pass a variable by reference, notice that the function
declaration itself does not indicate that references are allowed. There is
a configuration directive allow_call_time_pass_reference
which will allow you to ignore this error. However, in future versions of
PHP, the chances are good that your code will break. It is important to
make sure that when passing variables by reference that you declare the
function as accepting references:
function myfunc(&$myvar) {
$myvar = 1;
}Tip 2: Avoid using function return values directly
Another common stylistic error which occurs among both seasoned and new PHP developers alike is the tendency to use return values from functions directly as parameters for other functions. Consider the following code:
<?php
$mystring = "Test String";
$my_variable = (strcmp(strtoupper(substr($mystring,
(strlen($mystring) > 10) ? strlen($mystring) - 10 : 0)),
strtoupper(substr($mystring,
rand(0,strlen($mystring))))) == 0) ? true : false;
?>
Writing code like this works, but can you tell me what it does? Furthermore, would anyone looking at this code who didn't actually write it have any idea? These factors should both influence the way you write your scripts. Here's a much easier to read version of the above code:
<?php
$str_len = strlen($mystring);
$start_pos = ($str_len > 10) ? $str_len - 10 : 0;
$start_rand = rand(0, $str_len);
$substr_rand = substr($mystring, $start_rand);
$substr_base = substr($mystring, $start_pos);
$substr_rand = strtoupper($substr_rand);
$substr_base = strtoupper($substr_base);
if(strcmp($substr_base, $substr_rand) == 0) {
$my_variable = true;
} else {
$my_variable = false;
}
?>Now can you tell what this code does? The point is to not be afraid to write a few extra lines of code for the sake of readability. In the long run it will most definitely pay off. If you really feel the urge to use return values directly, as a general rule of thumb, if the combined number of parameters between all of the functions is three or less, it is acceptable in most circumstances:
<?php
/* "acceptable", but not recommended */
$mystr = substr(strtoupper($myvar), 5);
?>
In this case, we are calling the substr() function with
only two parameters, one of which is the strtoupper()
function which accepts one parameter. Since there are a total of three
parameters in this statement, it's probably acceptable to combine them
into a single line if so desired.
Although there are times when using a return value directly in another function may be acceptable, please note that one should never use a conditional assignment statement as function call parameter:
<?php
/* Don't ever do this */
$mystr = substr(strtoupper($myvar), (strlen($myvar) > 10) ?
strlen($myvar) - 10 : 0);
?>Tip 3: Avoid using improper array syntax
One common syntax mistake is found when working with string-indexed arrays. Due to the nature of PHP, a piece of code such as the following:
<?php
$myarray = array('foo'=>'Your index was foo', 'bar'=>'Your index was bar');
echo $myarray[bar];
?>will, as you might expect, print Your index was bar to the
browser. However, if the following nearly identical script is executed,
things will break:
<?php
define('bar', 'foo');
$myarray = array('foo'=>'Your index was foo', 'bar'=>'Your index was bar');
echo $myarray[bar];
?>Instead of the expected output, the script will now display Your
index was foo to the browser. Since in this case I have defined a
constant bar whose value is foo, PHP has
replaced each instance of bar with the string
foo in my script. Since my array index is not represented
explicitly as a string (being encapsulated in single or double quotes), it
is automatically replaced with the value foo. My
echo statement actually displays the value stored in the
array index defined by the constant (not the string) bar. In
order to prevent this confusing mess, it is important that all non-integer
array indexes be clearly defined as strings as shown:
echo $myarray['bar'];Even if you have no constants defined in your scripts, there is no assurance that from version to version of PHP a new constant won't be introduced which will cause undesired behavior. Note that there is one exception to this rule, namely, when referencing an array index from within a string. For instance, you are already familiar with the following syntax:
echo "My Value {$myarray['bar']}";where the value associated with the key bar in the array
$myarray will be displayed. Since this array is in a string,
this syntax can be simplified without consequence as shown:
echo "My Value $myarray[bar]";For consistency's sake, I recommend the former of these two examples be used (when working with objects and their member variables only the former will work). If nothing else, you should be aware of the differences.
Coming soon
That concludes the first of many columns in my PHP Paranoia series. You are already well on your way to writing quality PHP code, but we're not finished yet. There are still many more common mistakes made by PHP developers to discuss, so check back again soon for my next article.
John Coggeshall is a a PHP consultant and author who started losing sleep over PHP around five years ago.
Read more PHP Foundations columns.
Return to the PHP DevCenter.
You must be logged in to the O'Reilly Network to post a talkback.
Showing messages 1 through 5 of 5.
-
great tips!
2007-10-15 21:07:38 emailroy2002 [Reply | View]
good job, you really explained it such a way i can relate!!! mabuhay!
-
Tip 2 Not Really a Problem
2003-12-14 23:39:40 anonymous2 [Reply | View]
I would argue that a reasonable programmer would be able to understand your example. Sure, make your code clean and readable using blank lines, indents and so on. Perhaps you don't need to use the ternary operator either in your first example, but any programmer worth his / her salt should be able to read this.
If not, then try a different vocation / hobby.
-
style rules
2003-06-14 16:59:40 anonymous2 [Reply | View]
No need to fussily always use the ugly <?php and other default values. To make your code more transportable write an include file that sets all the php config directives to what your app needs. If your code can't change them it can check and spit out an error.
<?php
ini_set('short_open_tag', 1);
assert(ini_get('short_open_tag') == 1);
?>
I think it's a good idea to do this for any significant PHP application, rather than rely on whatever happens to be in the php.ini file when you wrote the code, especially in a shared hosting environment.
As for chaining function calls, years of experience with Lisp and C -- both much older than PHP -- show that proliferating temporary variables is an even worse idea. The author wrote a deliberately ugly and obfuscated example, but with some indenting the code can be perfectly obvious. Anyone who can't follow a few nested function calls should not be writing important code anyway.
Try writing in a language that doesn't have functions that return values and you'll quickly see how tedious that can be.
The ternary operator condition ? expression : expression lends itself to misuse, but there's nothing inherently wrong with using it inside a function call or anywhere else. The order of evaluation is well-defined. Sometimes simply enclosing the whole thing in parentheses makes it clearer. Again indenting and clarity in writing go a long way.
Greg Jorgensen
-
Style mistakes
2003-06-13 02:53:17 anonymous2 [Reply | View]
Chaining function calls is not a mistake, but is good technique, as long as the functions have meaningful names, and your layout is good....
$self->showPage( 'template.tmpl',
array(
'trees' => $self->getTreesByType( $type ),
'bushes' => $self->getBushesByType( $type )
)
);
$self->addToBasket(
$self->getProductID( $style, $colour, $size ),
$quantity
);
-
E_ALL error reporting is a good thing
2003-05-30 07:14:03 anonymous2 [Reply | View]
An additional hint: If you set error_reporting to E_ALL (via php.ini, .htaccess, or ini_set() in your script) many "style mistakes" like these (though not all of these) will be flagged for you. It's a great way to make sure you are not perpetuating deprecated syntax in your scripts. Running my dev server that way has cleaned up my code a lot -- for example, if I reference a variable without assigning something to it first, I get an error message. That one alone has saved me hours of debugging time.
--pbx
