When writing a function it's important to highlight the "main line" of the function if you can. Many times you'll have a "guard" at the beginning of the function to test for appropriate values, or to check for the existence of a file or enough disk space, or whatever. Here's an example from some of my own code I just modified. The original code was something like:
<?php
function & loadSettings($settings_path){
global $KISML;
$temp = array();
if(!file_exists($settings_path)){
header('Content-type: text/plain');
die("Settings file not found!");
}else{
$f = fopen($settings_path, 'rb') or die("Couldn't open settings file");
$temp = &$KISML->load($f);
fclose($f);
}
return $temp;
}
?>
By removing the else block, you make it clearer that the beginning if clause is simply a "guard" and not part of the logic of your code. You therefore highlight the "main line".
<?php
function & loadSettings($settings_path){
global $KISML;
if(!file_exists($settings_path)){
header('Content-type: text/plain');
die("Settings file not found!");
}
$f = fopen($settings_path, 'rb') or die("Couldn't open settings file");
$temp = &$kisml->load($f);
fclose($f);
return $temp;
}
?>
It's important to point out that there were things in the original function that didn't have to be there, such as the initial $temp = array() line and the returning of $temp outside of the else block. It's embarrassing that those things were there in the first place, but it makes my point stronger. Those superfluities were naturally eliminated when the else block was eliminated and the "main line" was highlighted.
Another example of eliminating else blocks and highlighting the main line is something like this (this isn't real code, I'm making it up on the spot, though it happens to use the function above):
<?php
function getSettings(){
if(foo){
$settings = loadSettings();
#do other stuff
}else{
$settings = array();
}
return $settings;
}
?>
It's better to highlight the main line and get rid of the else block:
<?php
function getSettings(){
if(!foo) return array();
$settings = loadSettings();
#do other stuff
return $settings;
}
?>
Here's a final example from real code I'm working on now. This is the code that prints the timing data on the lower-right-hand corner of this page. getmicrotime() is the same as microtime(true) in PHP 5.
<?php
function timer($finish = false){
static $time;
if($finish)
echo '<p style="font-size: smaller">This page took about ',round(getmicrotime()-$time,4),' seconds to generate.</p>';
else
$time = getmicrotime();
}
?>
The utility of this example is questionable. It's very short, and (therefore) is one of the few cases where using this "trick" actually winds up making the code longer (just one line longer), but I think I'm going to stick with this updated version of the code because it's clearer:
<?php
function timer($finish = false){
static $time;
if($finish){
echo '<p style="font-size: smaller">This page took about ',round(getmicrotime()-$time,4),' seconds to generate.</p>';
return;
}
$time = getmicrotime();
}
?>
(P.S. I really gotta get another language's syntax highlighter programmed in here so I can stop using PHP for all my code examples 
)
Update (July 26): Here's one more example from real code I'm going through right now. Before:
<?php
function setSize($field_name, $size){
if(isset($this->field_settings[$field_name])){
$this->field_settings[$field_name]['size'] = $size;
return true;
}else{
return false;
}
}
?>
After:
<?php
function setSize($field_name, $size){
if(!isset($this->field_settings[$field_name])) return false;
$this->field_settings[$field_name]['size'] = $size;
return true;
}
?>
You save 3 lines and the condition at the top is more clearly a "guard".
And, here's one more. Before:
<?php
function disabled($bool = NULL){
if(isset($this->field_settings[$field_name])){
if(is_null($bool)){
return $this->field_settings[$field_name]['disabled'];
}else{
$this->field_settings[$field_name]['disabled'] = $bool;
return true;
}
}
return false;
}
?>
After:
<?php
function disabled($bool = NULL){
if(!isset($this->field_settings[$field_name])) return false;
if(is_null($bool)) return $this->field_settings[$field_name]['disabled'];
$this->field_settings[$field_name]['disabled'] = $bool;
return true;
}
?>
Friday, July 23, 2004
Permalink
Since some time I use such style of coding (I like it), but I always thought it is a bad practice. Using many return, continue, break statements makes the code more unreadable ? Something like goto statements ? You don't know what exactly happens in the code and with if/else there is no such problem.