Ideas for simplificaton - Logic to choose which view to load

Question

I have a situation where depending on what the value of a uri segment is, my controller will load a different view. Here's what the code looks like in part:

          //which view do we load?
          if (strpos(strtoupper($data['hardwaremodel']),"HP") !== false)
          {
          //hp view should be loaded
          $data['main_content']='hpad';
          }
          elseif (strpos(strtoupper($data['hardwaremodel']),"DELL") !== false)
          {
          $data['main_content']='dellad';       
          }
          //load view.
          $this->load->view('includes/template', $data);    

For many methods in my controller, I have similar logic. So another method would have the exact same code, but the names of the view would be different dpending on what action the user requested. So for example, instead of loading the Dell address page above ('dellad') it would load the Dell status page ('dellstatus')

I'm wondering if I change my logic so that all the methods in the controller call a new function that determines what view to load for them will simplify things.

I think the function would have to look something like: (just pseudocode)

 private function loadview($callingfunctionname, $hardwaremodel)
 {              
 switch ($callingfunctionname)
 {
     case 'functionA':
           if hardware = dell {
               return "viewabc"
           }
           else {
              return "viewdef"
            }
           break;
     case 'functionB':
           if hardware = dell {
               return "view123"
           }
           else {
              return "view456"
            }
           break;               
    }//end switch
   } 

But I don't know that it will really simplify things to it this way. Can you help me improve this code? the other point is that eventually, I will end up with more than 2 hardware types that I have to check for.

Thanks.

Answer 1

Based on your answer, you should put the mapping in an array and create a for each loop. Maybe it would also be a good idea to extract the strtoupper($hardwarename) in a local variable that you don't do it in any if in the worst case. In addition to that you missed to add a default case, which might lead to an error in loading the template. Furthermore you have to move the longer DELL check in front of the shorter otherwise you will never reach this branch.

private function checkhardwarename($hardwarename)
{        
    $mapping=array("HP"=>"hp",
                   "DELL%20SF"=>"dell_sf",   
                   "DELL"=>"dell");
    $hardwarename=strtoupper($hardwarename);

    foreach ($mapping as $needle=>$viewname) 
    {
        if (strpos($hardwarename, $needle) !== false) return $viewname;
    }

    return "default";
}

Answer 2

I think I've come up with something cleaner:

code in controller methods simplified to this:

    //which view do we load?
    $viewnamePrefix = $this->checkhardwarename($data['hardwaremodel']);
    $data['main_content']=$viewnamePrefix.'ad';
    $this->load->view('includes/template', $data);      

and then my new function to check hardware name:

private function checkhardwarename($hardwarename)
{        
        if (strpos(strtoupper($hardwarename),"HP") !== false)
          {
          return 'hp';
          }
          elseif (strpos(strtoupper($hardwarename),"DELL") !== false)
          {          
              return 'dell';
          }
          elseif (strpos(strtoupper($hardwarename),"DELL%20SF") !== false)
          {
              return 'dell_sf';
          }
}

Answer 3

I'd introduce a function that computes the data, so that you encapsulate all the rules there.

$data['main_content']=loadMainContent($data['hardwaremodel']));

Finally to improve the clarity you should incapsulate the check in a function in order to replace this line with strpos(strtoupper($data['hardwaremodel']),"HP") with something cleaner like checkHardwareModel($data['hardwaremodel']),"HP").