1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27
public function setClass($class){
switch($class){
case "050":
case "055":
case "060":
case "065":
case "070":
case "077":
case "085":
case "092":
case "100":
case "110":
case "125":
case "150":
case "175":
case "200":
case "250":
case "300":
$this->class = $class;
break;
default:
print "invalid class!";
return false;
}
return true;
}
Refactorings
No refactoring yet !
Corey Grusden
August 12, 2008, August 12, 2008 19:04, permalink
It's a setter, so there's not really a compelling reason to return anything. There's a few other fundamental issues with this code, but here's a refactoring to maybe help clean it up and make it easier to add/remove classes in the near future without having to touch the function again.
Add the $CLASSES_CONSTANT to a global environment file and also rename it if need be, this way you can do the add/remove in 1 spot.
1 2 3 4 5 6 7 8 9 10 11 12 13
$CLASSES_CONSTANT = array("050","055","060");
function setClass($class)
{
if(in_array($class, $CLASSES_CONSTANT))
{
$this->class = $class;
}
else
{
print "invalid class!";
}
}
Marco Kranenburg
August 12, 2008, August 12, 2008 19:06, permalink
I would use the in_array() function to avoid the long listing and repeated typing:
1 2 3 4 5 6 7 8 9 10 11
public function setClass($class){
$my_array = array("050", "055", "060", "065", "070", "077", "085", "092", "100", "110", "125", "150", "175", "200", "250", "300");
if (in_array($class, $my_array)){
$this->class = $class;
break;
} else {
print "invalid class!";
return false;
}
return true;
}
cmcculloh
August 12, 2008, August 12, 2008 19:45, permalink
Hrm... not a bad idea. Thanks!
Mario Estrada
September 11, 2008, September 11, 2008 03:00, permalink
Although I think your code is much faster that using in_array().
Mark
October 9, 2008, October 09, 2008 02:54, permalink
Well since you are using 'this' you must already be a class so you would have to use a const. Depending on your use case maybe you could even do a check at the constructor to see if it a valid class...here are a few ideas of what I would do to allow the most flexiblity and reusability.
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25
//somwhere in your class
const CLASSES = {021,078,192,210,240};
//constructor
function __construct($class){
$this->setClass($class);
}
function setClass($class){
if($this->isValidClass($class)
$this->class = $class;
return true;
else
return false;
}
private function isValidClass($class){
return in_array($class,SELF::CLASSES);
}
I'm making a switch statement to verify some input. It's got all of the possible values (not likely to change much) and if the passed item isn't one of these values it prints an error. Is this a good idea? Is there a better way? Thanks!