Howdy folks,
Sorry for not getting back to you sooner. Between speaking engagements, framework releases and hard drive crashes, blog posting just gets lost in all the chaos sometimes. You know how it is.
So, today's code was randomly selected from the entries I've received since the introductory post. The entire submission is actually two files, which together comprise a console script for interacting with a web service (the names of the entrants have been withheld to protect the identities of the guilty). Rather than examine the full entry, we're going to take a look at one part which I find comes up fairly often: switch block overkill.
Don't get me wrong, switch blocks are great, especially for flow control (which makes the title of this post slightly misleading and sensationalist), but they're often overused in ways that make your code less maintainable. Let's take a look at the example:
for($k=
0;
$k<count
($args);
$k++
) {
$option =
$args[$k];
switch ($option) {
case 'id':
if (mb_ereg("^[0-9 ,]+$",
$args[$k+1])) {
$this->
id =
explode(',',
str_replace(' ',
'',
$args[$k+1]));
$k++;
} else {
$err =
'No id number found with id option';
}
break;
case 'q':
$this->
verbose =
0;
$this->
quiet =
true;
break;
case 'v':
$this->
verbose =
1;
$this->
quiet =
false;
break;
case 'V':
$this->
verbose =
2;
$this->
quiet =
false;
break;
case 'D':
$this->
debug =
true;
$this->
verbose =
2;
$this->
quiet =
false;
break;
case 'T':
$this->
testrun =
true;
$this->
quiet =
false;
break;
case 'L':
if (mb_ereg("^[0-9]+$",
$args[$k+1])) {
$this->
limit =
$args[$k+1];
$k++;
} else {
$err =
'Limit param can only be a digit';
}
break;
default:
if (!
isset($valid_args[$option])) {
$err =
'Uknown argument \''.
$option.
'\'.';
} else if ($valid_args[$option] !==
false) {
$k++;
}
break;
}
}
This code takes a list of arguments from the command line, and uses them to set the state of the shell object. In other words, it is creating a mapping between the parameter flags and the shell object's properties.
A key to writing flexible, maintainable code is realizing that your applications are basically comprised of two things: algorithms and data. Algorithms are the code, the logic. Data is what the algorithms operate on, though in many cases (like this one), data is showing up as part of the algorithm: it's too tightly bound up in logic, and unnecessarily so.
Finding where data is tied up in your business logic can sometimes be subtle and hard to spot. It might help to think about the distinction in these terms. Logic (or algorithm) code does something: it causes a transition to the next state of execution within your application. Conditionals (if/switch blocks) branch, loops iterate, etc. Conversely, data code is declarative, it takes no direct action; i.e. setting a variable or object property. (Unless __set()
is involved, in which case you're invoking a method anyway, so it doesn't count. Cheater.)
Now, setting a variable does cause a change of state within the machine of your application, but in and of itself, this change has no value. It requires some form of processing via logic code. With that in mind, let's take a look at the re-written example:
$flags =
array(
'q' =>
array('verbose' =>
0,
'quiet' =>
true),
'v' =>
array('verbose' =>
1,
'quiet' =>
false),
'V' =>
array('verbose' =>
2,
'quiet' =>
false),
'D' =>
array('debug' =>
true,
'verbose' =>
2,
'quiet' =>
false),
'T' =>
array('testrun' =>
true,
'quiet' =>
false)
);
for ($k =
0;
$k <
count($args);
$k++
) {
$option =
$args[$k];
$value =
$args[$k +
1];
switch (true) {
case ($option ==
'id'):
if (mb_ereg("^[0-9 ,]+$",
$value)) {
$this->
id =
explode(',',
str_replace(' ',
'',
$value));
$k++;
} else {
$err =
'No id number found with id option';
}
break;
case (isset($flags[$option])):
foreach ($flags[$option] as $key =>
$val) {
$this->
{$key} =
$val;
}
break;
case ($option ==
'L'):
if (mb_ereg("^[0-9]+$",
$value)) {
$this->
limit =
$value;
$k++;
} else {
$err =
'Limit param can only be a digit';
}
break;
default:
if (!
isset($validArgs[$option])) {
$err =
'Uknown argument \'' .
$option .
'\'.';
} else if ($validArgs[$option] !==
false) {
$k++;
}
break;
}
}
First of all, notice how the code formatting has been changed: things are now spaced out in a more consistent format that's easier to read, and an explaining variable ($value
) has been added alongside $option
, to make it more clear from the code which options support parameters, and which are stand-alone flags. When you're writing code, even if you're in a hurry, it always helps to think of the next guy. That next guy might be you in a couple months (or days) when you come back to the code and wonder what the heck you were doing there.
Moving on: as we can see, the data mapping has been abstracted out of the logic and put at the top of the code block. This means that (a) our data can expand infinitely with no logic duplication, and (b) the data can be moved around, manipulated, re-used, or loaded from an external source. This highlights another property of logic vs. data: logic is fixed and immutable, while data is infinitely flexible.
Okay... any questions/comments/snide remarks?
No? Good.
Now get outta here and refactor your code!