Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite
1

I'm just wondering if you see any ways I could simplify the following code without making it too difficult for a newbie to understand what I'm trying to do:

    if (trim($this->uri->segment(5)) == 1) {


    if ( $this->my_model->functionA($this->uri->segment(8),$this->uri->segment(4)) )
    {
        $data['light'] =  $this->my_model->data();
    }
    else
    {
        show_error($this->my_model->errormessage());
    }
    }
    elseif (trim($this->uri->segment(5)) == 0) {

      if ( $this->my_model->functionB($this->uri->segment(8),$this->uri->segment(4)) )
      {
          $data['light'] = $this->my_model->data();
      }
      else
      {
          show_error($this->my_model->errormessage());
      }
    }       
    elseif (trim($this->uri->segment(5)) == 2) {

      if ( $this->my_model->functionC($this->uri->segment(8),$this->uri->segment(4)) )
      {

          $data['light'] = $this->my_model->data();
      }
      else
      {
          show_error($this->my_model->errormessage());
      }        
    }

I guess I could use a switch statement but i would still need to check the results of each function call. Would it still be beneficial to use switch? if so, can you explain why?

Any suggestions would be appreciated. thanks.

share|improve this question

2 Answers

up vote 3 down vote accepted

I haven't been able to test it, but I'm fairly sure this will work:

$functionName = 'function';
switch(trim($this->uri->segment(5)))
{
    case 1: $functionName .= 'A'; break;
    case 0: $functionName .= 'B'; break;
    case 2: $functionName .= 'C'; break;
}
if($this->my_model->$functionName($this->uri->segment(8), $this->uri->segment(4)))
{
    $data['light'] = $this->my_model->data();
}
else
{
    show_error($this->my_model->errormessage());
}

For more information, see the PHP page on Variable Functions.

share|improve this answer
this is perfect! thanks for the great suggestion – dot Jan 9 at 13:56
@dot: The problem with this solution is the use of variable-functions. These are frowned upon for their lack of clarity. Not only will "newbies" have problems with this code, seasoned programmers will as well. There is no way to know which functions are being called without having to track it down manually. You can't even rely on your IDE to help you out. My advice: stay clear of variable-functions and variable-variables. While this looks clever and clean, its a nightmare to debug. No offense to MrLore, but I'd lean more towards Ryan's answer simply for clarity's sake. – mseancole Jan 9 at 21:50
@mseancole There is nothing wrong with variable functions. They are easier than reflection for basic and efficient tasks, and if your code is complex as a result of using them, I recommend commenting your code for yourself and others. That is what 'seasoned programmers' do. – MrLore Jan 9 at 22:02
There is everything wrong with variable-functions. As I already explained they subtract from your code's legibility and are potential debugging nightmares. Adding inline comments to try and cover it up just adds clutter. Better to expand it, as Ryan did, or better yet, leave it up to the method to decide what to do. I have no idea what the difference between functionA(), *B(), and *C() are, but they are likely so similar that they could be combined and a new parameter passed to distinguish them. functionX( $seg8, $seg4, $diff ). – mseancole Jan 9 at 22:31
@mseancole That is a problem with function and variable naming, not the methodology. Edit: And I had assumed they were placeholders to obscure the source. – MrLore Jan 9 at 22:34
up vote 2 down vote

Don't call the same function over and over with the same parameters, you won't get different results so why call it again and again? Also I would use a switch.

<?php

$seg5  = trim($this->uri->segment(5));
$seg8 = $this->uri->segment(8);
$seg4 = $this->uri->segment(4);

switch($seg5){
    case 0:
        if($this->my_model->functionB($seg8, $seg4)){
            $data['light'] = $this->my_model->data();
        }else{
            show_error($this->my_model->errormessage());
        }
        break;
    case 1:
        if($this->my_model->functionA($seg8, $seg4)){
            $data['light'] = $this->my_model->data();
        }else{
            show_error($this->my_model->errormessage());
        }
        break;
    case 2:
        if($this->my_model->functionC($seg8, $seg4)){
            $data['light'] = $this->my_model->data();
        }else{
            show_error($this->my_model->errormessage());
        }
        break;
}
share|improve this answer
Thanks. I'll implement that suggestion too. But as far as really simplifying the code, I think MrLore's answer is what I was after. – dot Jan 9 at 13:57
My only suggestion to this answer would be to move that repeated else statement out of the switch. After the switch you can check if $data[ 'light' ] has been set and use that as your else statement. I especially like that you abstracted those segments. It removes the complexity from the statements. – mseancole Jan 9 at 21:51

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.