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(); |
This site is a digital habitat of Sameer Borate, a freelance web developer working in PHP, MySQL and WordPress. I also provide web scraping services, website design and development and integration of various Open Source API's. Contact me at metapix[at]gmail.com for any new project requirements and price quotes.
Get the latest posts
10 Responses
1
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);
}
2
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.
3
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 [...]
4
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.
5
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.
6
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
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..
7
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.
9
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.