Should I refactor this piece of code?
Posted on 9/11/06 by Felix Geisendörfer
Most of you will probably be familiar with the concept of refactoring. It is the process of modifying an exisiting piece of code in order to improve it's readibility or structure while preserving it's original meaning or behavior. Automated testing makes refactoring a lot easier, as you can see if the changes you make to the code break it's original functionality. Anyway, this post was actually going to be about a little Helper function I wrote and felt like sharing and not about refactoring. But as it's the case with most of my code, the first step before blogging about it, is to refactor it for readability. But often I also tend to refactor code on here to be more generic / reusable then it needs to be for my actual application to make it more attractive to a broader audience. This was also the case with the little Helper function that can be used to convert a given amount of bytes into a more meaningful string using the appropriate units. The end result was that I decided to use the resulting code as an example for refactoring instead.
First of all, consider the function I initially thought about posting:
{
function bytesToString($bytes, $round = 1)
{
if ($bytes==0)
return '0 bytes';
elseif ($bytes==1)
return '1 byte';
$units = array('bytes' => pow(2, 0),
'kb' => pow(2, 10),
'mb' => pow(2, 20),
'gb' => pow(2, 30),
'tb' => pow(2, 40),
'pb' => pow(2, 50),
'eb' => pow(2, 60),
'zb' => pow(2, 70),
'yb' => pow(2, 80));
$lastUnit = 'bytes';
foreach ($units as $unitName => $unitFactor)
{
if ($bytes >= $unitFactor)
$lastUnit = $unitName;
else
return round($bytes/$units[$lastUnit], $round).' '.$lastUnit;
}
}
}
Now even so I expect most of us to deal within units much smaller then 1 TB on the web, I added the units that come afterwards for completeness. But that's not really important. What's interesting is is that this function above serves a well defined purpose and there is no strong reason for not leaving it as is. However, when posting about it on here (so I thought), it should be more generic as it's part of an object called UnitsHelper. What happened was a ridiculous little refactoring session turning this innocent little function into being part of an automagic unitToString function that works like the Model::findBy<Field>() stuff in CakePHP. Only that in this case it's UnitsHelper::<unit>ToString(). Before explaining any further, take a look at the result:
{
function __call__($method, $param)
{
if (preg_match('/^(.+)toString$/i', $method, $match))
{
array_unshift($param, $match[1]);
return call_user_func_array(array(&$this, 'unitToString'), $param);
}
}
function unitToString($unit, $quanitity, $round = 1)
{
if ($quanitity==0)
return '0 '.$unit;
elseif ($quanitity==1)
return '1 '.Inflector::singularize($unit);
$unitFct = $unit.'Units';
if (!is_callable(array(&$this, $unitFct)))
return $quanitity.' '.$unit;
$units = $this->{$unitFct}();
foreach ($units as $unitName => $unitFactor)
{
if ($quanitity >= $unitFactor)
$lastUnit = $unitName;
else
return round($quanitity/$units[$lastUnit], $round).' '.$lastUnit;
}
}
function bytesUnits()
{
return array('bytes' => pow(2, 0),
'kb' => pow(2, 10),
'mb' => pow(2, 20),
'gb' => pow(2, 30),
'tb' => pow(2, 40),
'pb' => pow(2, 50),
'eb' => pow(2, 60),
'zb' => pow(2, 70),
'yb' => pow(2, 80));
}
}
First of all: Note that this code will only work in CakePHP 1.2 where Helper extends Overloadable and therefor makes it easy to implement automagic functions like the one used here.
Other then that this is good example to answer the question about when to refactor. What I did in this case was to refactor the function, so it would be easy to use it's core functionality for other (linear) unit systems as well. So if I was to expect that I will deal with lot's of units in the metric system in a project, this little refactoring would become very useful as the amount of used units grows. However, in the little experimental project I'm using this Helper in (a code review assistant) there probably won't ever be another unit then bytes.
So what does this mean? Well I think it's obvious: The principle of YAGNI (You Arent Gonna Need It) says that in my case I just wasted a lot of time. However, in the scope of somebody elses project who might reads this post, refactoring this Helper was very benifital. In general I'm in big favor of 37 Signals philosophy of not trying to be everything to everybody, and sticking with what you need. So just because there are a lot of benifits to staying DRY (Don't Repeat Yourself), this doesn't mean it should become your #1 priority. If you are unsure if you are going to need something, don't code it. If you are not sure if refactoring some piece of code will make the world a better place, don't do it. Try to get a feel of where the critical parts of your application are located and focus on making them the most beautiful code you can imagine (or afford in terms of time investment ^^). But don't try to be as insane and refactor the little bytesToString function of mine if all you need is a bytesToString function ... ; ).
Now even if so this post didn't answer all your questions about refactoring, it at least provides you with (one) solution to a common problem ; ).
--Felix Geisendörfer aka the_undefined