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.
Showing messages 1 through 9 of 9.
-
E_ALL error reporting is a good thing
2003-05-30 07:14:03 anonymous2 [View]
-
E_ALL error reporting is a good thing
2003-06-02 02:22:22 John Coggeshall |
[View]
Setting your error_reporting level to E_ALL is an excellent setting during development, as notices which are usually ignored are displayed.
However, as you correctly pointed out this is for a development server only! Once a site is in a production environment it is recommended error reporting be turned off completely. -
E_ALL error reporting is a good thing
2003-06-02 08:14:34 anonymous2 [View]
I couldn't agree more, it helps you write clean code, and catch misspellings in variables (especially case-sensitivity issues with variables).
$firstName = $theUser->firstName();
return $firstname;
This would generate a warning, because $firstname is undefined (should be $firstName).
Another frustrating thing is when evaluating open-source code which was NOT developed with error_reporting E_ALL, and there are hundreds of errors all over the place, and then I need to make a custom .htaccess file to disable errors for a certain dir, and generally have a bad impression of the software before I even try it out! So, it's definitely possible to write functioning code without error_reporting on strict mode, but keep in mind that other people who use your code might see this as sloppy.
-
Style mistakes
2003-06-13 02:53:17 anonymous2 [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
);
-
style rules
2003-06-14 16:59:40 anonymous2 [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 rules
2003-06-14 20:26:25 John Coggeshall |
[View]
Although your solution is valid, it's also costly to have to needlessly include an additional file every single request. By simply using the standard
Regarding chaining function calls, yes my example was very obfuscated. However, I stand by the 3-parameter rule as a solid method of doing things. I do agree that as easily as there can be too many chained function calls, you can have too many temporary variables.. Also, although I cannot speak in regards to Lisp I must point out that in this case a comparison to C isn't valid. In C variables must be declared, etc. and perhaps in that situation can be more trouble than it's worth.. however in PHP, where variables can be created and assigned values without such hassle the impact is minimal.
Finally, regarding the ternary operator.. I again disagree. :) Since my arguments against this parallel those I have already made regarding chaining function calls inappropriately, I won't repeat them again.
Perhaps one style mistake that I should have made a point to bring up is that readability is extremely important. Without being able to quickly ascertain what is going on with a piece of code the chances of missing or not finding bugs is that much greater. If you claim that a piece of code is completely readable with 20 function calls chained together (as long as you indent it properly) then so be it :). However, as a general rule of thumb the techniques I have discussed in this column are extremely important and I stand by each of them :)
Thanks for your feedback.
John
-
Tip 2 Not Really a Problem
2003-12-14 23:39:40 anonymous2 [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. -
Tip 2 Not Really a Problem
2005-01-29 09:36:52 blueeyedmick [View]
I'm glad I don't work with/for you. You obviously have no concern about wasting another person's time in order to save a few keystrokes.
The key here isn't "can I figure out this tangled mess of code (because I rule as a hacker!)", but rather "can I make this so the next guy can figure it out without any effort at all". Wasting others' time is a sin most companies will punish you for.
-
great tips!
2007-10-15 21:07:38 emailroy2002 [View]
good job, you really explained it such a way i can relate!!! mabuhay!
--pbx