Refactoring 1: Consolidating Conditional Expressions : CodeDiesel

Refactoring 1: Consolidating Conditional Expressions

Posted in: php, refactoring | Save to del.icio.us | Twit This! 4 Jan 2009

As said in the previous post, I’m kick-starting the refactoring series with the ‘Consolidate Conditional Expression’ pattern. Bear in mind that the examples that will be given are deliberately made simple to keep the refactoring ideas in focus. So lets start with our first pattern.

Consolidate Conditional Expressions
Many times you see a group of conditionals where the returned values are the same. To make the code cleaner you can group the conditionals together using the ‘&&’ or the ‘||’ operators and then extract the code into a separate function. This also has the added benefit that you can reuse the extracted method in other places where the required conditional goes. Before using this pattern make sure that the grouped conditionals do not have any side-effects; i.e they do not change any variables.

A simple example is shown below:

Before refactoring:

class Security {
 
	private $_isHttps = true;
	private $_isAdmin = true;
	private $_totalLoginsPerDay = 19;
	.
	.
	.
 
	public function can_upload_file()
	{
		if($this->_isHttps == false) return 0;
		if($this->_isAdmin == false) return 0;
		if($this->_totalLoginsPerDay > 100) return 0;
 
		return 1;	
	}
}
 
$login = new Security();
echo $login->can_upload_file();

After refactoring:

class Security {
 
	private $_isHttps = true;
	private $_isAdmin = true;
	private $_totalLoginsPerDay = 199;
	.
	.
	.
 
	public function can_upload_file()
	{
		return $this->check_security_status();
	}
 
	private function check_security_status()
	{
		if($this->_isHttps == false || 
		   $this->_isAdmin == false || 
		   $this->_totalLoginsPerDay > 100)
			return 0;
		else
			return 1;
	}
}
 
$login = new Security();
echo $login->can_upload_file();

Share this post

10 Responses

Wally

January 4th, 2009 at 11:58 am

this works and a bit shorter.
private function check_security_status() { return ($this->_isHttps === false || $this->_isAdmin === false || $this->_totalLoginsPerDay > 100); }

Matthew Weier O'Phinney

January 5th, 2009 at 9:49 am

Please, please, please don’t show if/then/else statements that do not use braces. Omitting braces is an easy way to slip into debugging hell later when you need to add additional statements, and is the reason why most public coding standards insist on braces for all conditionals.

Sameer Borate’s Blog: Refactoring 1: Consolidating Conditional Expressions : Dragonfly Networks

January 6th, 2009 at 3:58 am

[...] has posted the first article in his “Refactoring” series today - a look at boiling down conditional expressions to [...]

Bugz

January 6th, 2009 at 4:45 am

I have noticed that most people want to use refactoring to reduce the number of lines in their code, and even though most times there will be a code-volume reduction, the main objective of refactoring is to make the code more efficient, easier to handle and reusable.
When you refactor a project, you must aim at making the code the most reusable and simple as possible. Some times that will generate more lines of code than the original code, but it should be simpler to maintain.
Finally, one point that I think is very important to keep in mind is the fact that refactoring should be handled with the greatest care possible, documenting your project should in general, be the first step before refactoring anything to avoid refactoring you refactoring.

Les

January 6th, 2009 at 5:22 am

I know it’s only a short example but for the uneducated short examples are mostly meaningless.

For greater benefit, wouldn’t it be better to demonstrate with a real world example - actual application script with proper unit tests?

People would learn more and quickly if they saw how the refactor effects the wider application layer, but my concern can be noted not just for this blog but a 1000 others too.

It’s unfortunate.

lloyd27

January 6th, 2009 at 10:23 am

There are a few things i don’t like of this code..
The lacks of braces, the fact you’re returning 1 and 0 instead of true and false (which would be more semantically correct), the mixed usage of tabs and spaces to indent (spaces for life :D)..
Even the name of the method “check_security_status” is not fully semantically correct: isAdmin, or isSecure or something like that would be fine instead. A method that returns a boolean (or 1 and 0 in your case) IMHO should be called is… or can… or has…
I would refactor like this
public function can_upload_file() { return $this->_isAdmin and $this->isHttps and ($this->totalLoginsPerDay <= 100); }
This is more meaningful, easier to read and even shorter.. If you want you can separate in two different methods, but unless the new one is called somewhere else in the code, it would be pretty useless..

Joseph Gutierrez

January 7th, 2009 at 12:16 pm

Where are your unit tests? Any refactorings require unit tests. If you want to make sure you don’t change the behavior (logic) of your conditionals you need some unit tests.

sameer

January 7th, 2009 at 9:15 pm

I understand the importance of unit tests as mentioned in my earlier post ‘Refactoring: An introduction for PHP programmers’ but have left it out on purpose.

Joseph Gutierrez

January 8th, 2009 at 8:56 am

“…The important part is to test your code after refactoring.”

In this quote from your initial post do you write tests first, then refactor? Or do you write code then tests?

sameer

January 8th, 2009 at 10:10 am

Always write tests first and then refactor.