Refactoring 1: Consolidating Conditional Expressions

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();

10 thoughts on “Refactoring 1: Consolidating Conditional Expressions

  1. this works and a bit shorter.

    private function check_security_status() {
    return ($this->_isHttps === false ||
    $this->_isAdmin === false ||
    $this->_totalLoginsPerDay > 100);
    }

  2. 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.

  3. 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.

  4. 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.

  5. 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..

  6. 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.

  7. 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.

  8. “…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?

Comments are closed.