Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

share|improve this question

3 Answers 3

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";
}
share|improve this answer

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';
          }
}
share|improve this answer
    
Don't put your DELL%20SF conditional after the DELL conditional. strpos doesn't compare two strings, only locates the position of a given substring. Now, if a string contains DELL%20SF it certainly contains DELL too, so this means your DELL check will incorrectly flag DELL%20SF hardware too and the DELL%20SF condition will never be checked. –  Jason Larke Feb 19 '13 at 7:36

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").

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.