tags:

views:

124

answers:

5

How can I re-factor the following code to make it more concise and more maintainable?

if ($row['vocation'] == 1) $vocation = "sorcerer";
if ($row['vocation'] == 2) $vocation = "druid";
if ($row['vocation'] == 3) $vocation = "paladin";
if ($row['vocation'] == 4) $vocation = "knight";

if ($row['vocation'] == 5) $vocation = "master sorcerer";
if ($row['vocation'] == 6) $vocation = "elder druid";
if ($row['vocation'] == 7) $vocation = "royal paladin";
if ($row['vocation'] == 8) $vocation = "elite knight";
else $vocation = "none";
+14  A: 

I would recommend using an array, like this:

static $vocations = array(
  1 => 'sorceror',
  2 => 'druid',
  3 => 'paladin',
  4 => 'knight',
  5 => 'master sorceror',
  6 => 'elder druid',
  7 => 'royal paladin',
  8 => 'elite knight',
  );

$vocation = 
  isset($vocations[$row['vocation']]) ? $vocations[$row['vocation']] : 'none';
thomasrutter
This, with the added comment that if these are coming from a database, you might be better storing it in a Vocations table and joining it.
Mark
+1 Nice example, I'll cede this one to you ;]
Daniel LeCheminant
A: 

Start your project out right, use const now to represent those numeric constants and save yourself some headaches down the line. (in addition to using switch/case as others have suggested)

Trey
+2  A: 

Here's an example of used a switch to do this:

switch ($row['vocation']) {
    case 1:
        $vocation = "sorcerer";
        break;
    case 2: 
        $vocation = etc..
    default:
        $vocation = "none";
}

This is a common thing for many languages like C, Java, and C# and many others as well.

Tony k
+1 for the example, but nothing that messes up the readability of the code is an improvement, unless the cases become signficantly more complicated than setting $vocation.
Mark
+1  A: 

Following piece might be a little better. 8 elements are OK, but what if the list was containing 1000.

$list = array("sorcerer", "druid", ...);

$vocation = "none";

if($row['vocation'] <= count($list)){
    $vocation = $list[$row['vocation'] - 1];
}
Burcu Dogan
A: 

I'd use the suggestion with array and I'd use constants to represent the integer values like this:

define('VOCATION_SORCEROR', 1);
define('VOCATION_DRUID',    2);
define('VOCATION_PALADIN',  3);

$vocations = array(
  VOCATION_SORCEROR => 'sorceror',
  VOCATION_DRUID =>    'druid',
  VOCATION_PALADIN =>  'paladin'
);