name that month: adventures in refactoring
One day, at the place I was working at the time, I was debugging some legacy PHP code from a predecessor when I found this:
I've changed a few details, but the function really did contain twelve if() statements, and no else() clause. The function checked for all twelve months, regardless of which month was passed to it. The variable $month is evidently coming from something that is zero-based, and I'm guessing the poor programmer just couldn't wrap his/her brain around this:
Now this wasn't the function I was tasked with debugging. The correct month abbreviation actually printed on the web page, so the code wasn't absolutely wrong. Still, I couldn't muster the will to let it go.
The most trivial improvement would be to change each if() except the first one to elseif(). That way, at least, the function could stop checking the value of $month when it found the correct month. But we can do even better than that.
A marginally better solution would be to replace the if() with a switch() statement:
This suffers from the same limitations of checking every value sequentially, but it removes the twelve == operators, and the possibility of accidentally typing one as a single = and returning the wrong month. On the other hand, it opens the possibility of omitting a break; and returning the wrong month.
A better solution is this:
With one stroke we've removed the unnecessary incrementing of $month and the multiple comparisons.
On top of that, the $names array was already declared as a global variable at the top of the file. I didn't even need to make a local variable, although it's better to do so. Defining it locally ensures that changes to the global variable won't adversely affect this function.
So that's the solution I used, but could I have done even better? Many languages have a library function to return the month name or abbreviation, given the month number. PHP is not my primary language, but I'd be willing to bet that somewhere among PHP's 4376* built-in functions is one to handle this.
* estimated
function get_month_name ($month)
{
$month = $month + 1;
if ($month == 1) {
$name = 'Jan';
}
if ($month == 2) {
$name = 'Feb';
}
if ($month == 3) {
$name = 'Mar';
}
if ($month == 4) {
$name = 'Apr';
}
if ($month == 5) {
$name = 'May';
}
if ($month == 6) {
$name = 'Jun';
}
if ($month == 7) {
$name = 'Jul';
}
if ($month == 8) {
$name = 'Aug';
}
if ($month == 9) {
$name = 'Sep';
}
if ($month == 10) {
$name = 'Oct';
}
if ($month == 11) {
$name = 'Nov';
}
if ($month == 12) {
$name = 'Dec';
}
return $name;
}
I've changed a few details, but the function really did contain twelve if() statements, and no else() clause. The function checked for all twelve months, regardless of which month was passed to it. The variable $month is evidently coming from something that is zero-based, and I'm guessing the poor programmer just couldn't wrap his/her brain around this:
if($month == 2) {
$name = 'Mar';
}
Now this wasn't the function I was tasked with debugging. The correct month abbreviation actually printed on the web page, so the code wasn't absolutely wrong. Still, I couldn't muster the will to let it go.
The most trivial improvement would be to change each if() except the first one to elseif(). That way, at least, the function could stop checking the value of $month when it found the correct month. But we can do even better than that.
A marginally better solution would be to replace the if() with a switch() statement:
function get_month_name ($month)
{
$month++;
switch ($month) {
case 1:
$name = 'Jan';
break;
case 2:
$name = 'Feb';
break;
case 3:
$name = 'Mar';
break;
case 4:
$name = 'Apr';
break;
//et cetera
}
}
This suffers from the same limitations of checking every value sequentially, but it removes the twelve == operators, and the possibility of accidentally typing one as a single = and returning the wrong month. On the other hand, it opens the possibility of omitting a break; and returning the wrong month.
A better solution is this:
function get_month_name ($month)
{
$names = array ('Jan', 'Feb', 'Mar',
'Apr', 'May', 'Jun',
'Jul', 'Aug', 'Sep',
'Oct', 'Nov', 'Dec');
return $names[$month];
}
With one stroke we've removed the unnecessary incrementing of $month and the multiple comparisons.
On top of that, the $names array was already declared as a global variable at the top of the file. I didn't even need to make a local variable, although it's better to do so. Defining it locally ensures that changes to the global variable won't adversely affect this function.
So that's the solution I used, but could I have done even better? Many languages have a library function to return the month name or abbreviation, given the month number. PHP is not my primary language, but I'd be willing to bet that somewhere among PHP's 4376
Labels: functions, PHP, refactoring
0 Comments:
Post a Comment
Links to this post:
Create a Link
<< Home