views:

141

answers:

4

I have an internal application in which one function is containing too many switch cases.

This is developed in php.This specific function is used to write changes to database and also to keep a history of individual field values. So what it does is have a case for each field as different things needs to be applied for different fields.

switch ($item){
case 'item1':
do_something();
case 'item2':
do_something_different():
}

Is there a design pattern to follow in such cases. A function for each item doesn't look so future proof either.

Update: pastebin link

Thanks.

+1  A: 

You're not really revealing a lot about your code with that sample, but another option would be to use a state machine. You can take an array and assign the item names as keys and the values would be the function.

$machine = array('item1' => do_something, 'item_2' => do_something_different);
$machine['item1']();

Then it's even open for pluggability where you can include a file that says:

$machine['item3'] = do_something_else_else;
Matt Williamson
It should be noted that in a real state machine, each of those function will return the next state or the next function to run.
Matt Williamson
Ironically, I've seen state machines implemented with a switch statement.
JohnB
Your example is not really a state machine by itself, but otherwise a valid alternative to the given example above. A state machine could easily be implemented as switch-case statements, functions, objects or whatever suits you and your project.
Ivar Bonsaksen
+7  A: 

That's just not a good function. It should be three functions, edit_name, edit_manager and edit_liscencedata. you can move all the stuff that repeats between cases into the constructor of the Change class that you should define.

aaronasterling
I have to agree, now that he's posted the code. Not only will it be more clear, it will also execute faster since it will be a symbol lookup, rather than conditional checks.
Matt Williamson
I think I will go this route as this is not in production yet, I can fix it in couple of hours. Wanted opinion before its too late. Thanks
saint
+1  A: 

From the code snippet provided it's obvious you're facing quite a severe case of intermingled responsibilities and copy paste coding from lack of factoring. Your edit function handles stuff on at least three different layers and for three distinct objects. The refactoring operations I would be likely to apply are extracting Validator classes for each of the different sets of data you're handling, Model classes likewise for interacting with the database in each case and Commands to bind the three together: accept data, pass it through validators, then interact with their respective Models to persist the data. The Models would then generate Change entries based on successful operations.

If you're disinclined to take on such significant change right now, good first steps would be separating the different parts in your edit function into other, better partitioned functions and only worry about classes and polymorphism when you've tackled that first hurdle. To better understand why your code is broken and what you can do to mend it, I recommend reading something like Refactoring (by Martin Fowler) or perhaps The Pragmatic Programmer.

Ezku
I understand what you mean, this was made to test database and glue the visual templates. I wanted community opinion before starting production code. Validation part will be completely removed, meaning it wont be hard coded into it. Thanks
saint
A: 

Just a thought, but instead of the switch statement, you might want to have a sort of hook system available for this ...

so, for a list of possible stuff like

item1, item2, ..., itemN

you might try something like

function item1action ( ) { /* ... */ }
function item2action ( ) { /* ... */ }
function itemNaction ( ) { /* ... */ }

and have some sort of default method like

function itemDefaultAction ( ) { /* ... */ }

so that for the previous switch statement, you could instead do

$function = "{$item}action";
if (! function_exists($function)) {
    return itemDefaultAction();
}
call_user_func($function);
return call_user_func($function)
Cory Collier