Saturday, February 26, 2011

Returning the null value from a method

This example demonstrates the drawbacks of returning null from your methods. I allays preach to my colleagues that when something is wrong, your method should throw an exception instead of returning a null.
But sometimes, especially if I'm writing some tools for myself, because of the laziness, I use the return null too.

Few days ago I done that, and very soon I released that it was mistake which caused me unnecessary problems.

Story goes like this: There is an initial list of ids and result of a process is a array of fully populated objects. I use that procedure on two places in my tool: for generating RSS file and for generating a view script. The tool is written in Zend Framework, it uses a log file, when exception occurs displays a customized error page and sends a email to me.

Initially it was all flawless, but at some point, I released that I'm not able to read data for some of initial ids. So I modified a loading method in something like this:

public function loadItem( $id)
{
	$raw_data	=	$this->_getRawData( $id);
 	if ($this->_testRawData( $raw_data))
   		return $this->_parseData( $raw_data); 
 	return null;
}

Accordingly, I modified the controller part to:

public function rssAction() 
{   
	$ids	=	$this->getRssIds();   
 	$items 	= 	array();  
 	foreach ($ids as $id)   
 	{
  		$item = $this->manager->loadItem( $id);
  		if ($item)      
   			$items[] = $item;
  	}   
 	$this->view->items = $items;
}

I done it, tested the RSS feature and it was as expected, all OK. I've done few more fixes, and at last, after some time, I wanted to see the preview too.
Cold shower. PHP notices, warnings, fatal error. Undefined variable, call to a method on null object.... Why, how, what the hell ...?
Standard debugging procedure: I'm checking the log file. Nothing to find there. Next is to check the view script and associated action. Soon I found that the problem was that I had null values in array which supposed to be populated with objects. I simply forgot that I was using the same method on two places.

public function previewAction() 
{  
	$ids 	= 	$this->getPreviewIds();   
	$items 	= 	array();  
	foreach ($ids as $id)      
 		$items[] = $this->manager->loadItem( $id);
	$this->view->items = $items; 
}

At the end, as I'm experienced developer and this was my "in house" tool, I found and fixed it very quickly (still bad solution: again I put a test if the returned object is null), but it raised a really good question: When you are doing a bigger scale project, where you can not have all the things in your head, what is the right solution and what would be the consequences?

First the correct code variant:

// somewhwere in my manager class

public function loadItem( $id) 
{ 
 	$raw_data	=	$this->_getRawData( $id); 
 	if ($this->_testRawData( $raw_data))
   		return $this->_parseData( $raw_data); 
 	throw new ItemNotFoundException( 'Item ['.$id.'] could not be loaded');
} 

// somewhwere in my controller class 
public function rssAction() 
{ 
 	$ids 	= 	$this->getRssIds();  
 	$items 	= 	array();   
 	foreach ($ids as $id)   
 	{
  		try      
  		{       
   			$items[] = $this->manager->loadItem( $id);    
  		}      
  		catch (ItemNotFoundException $e)
  		{}   
  }   
  $this->view->items = $items; 
}
Let's assume that I still miss to upgrade the previewAction() method. Now, instead of bunch of warnings, I'll have Exception. This Exception is important because
1. Debugging: It will be stored to my log file, and in the development/testing process I'll quickly determine where is the problem and what I have to fix (Front controller handles it)
2. Application consistency: If I forgot to test any other part of my application which might use that manager method, and I put that code on live installation, users will encounter the customized error screen, error will be logged, and I'll receive the alarm email. The error will not be able to pass unnoticed!