current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite
2

Is there any way to improve this code? It looks a bit ugly for me.

if($url_chunks_num == 1) {
    $this->controller = $this->pageData[0];
} elseif($url_chunks_num == 2) {
    $this->controller = $this->pageData[0];
    $this->action = $this->pageData[1];
} elseif($url_chunks_num > 2) {
    $this->controller = $this->pageData[0];
    $this->action = $this->pageData[1];
    $this->values = array_slice($this->pageData, 2);
}

I thought about nested conditions, but this option is ugly as well.

share|improve this question

4 Answers 4

up vote 7 down vote accepted

I agree with luiscubal about restructuring the code (with the one minor difference that I would use braces), however, I think you could make something a bit cleaner with array_shift.

$this->controller = array_shift($this->pageData);
$this->action = array_shift($this->pageData);
$this->values = $this->pageData;

This is functionally equivalent to the code luiscubal posted, though his does not define defaults. (The two strings would default to NULL and then $this->values would default to array().)

This does change $this->pageData though, so if you need to keep that data around (which is implied by it being a property), you'll need to either use luiscubal's approach or create a temporary copy.

Since the array is small, and PHP is typically copy-on-write, making a copy shouldn't be an issue, however, if you're worried, you could take a non-direct route to basically have the same amount of copying as the if trees have:

$this->values = $this->pageData;
$this->controller = array_shift($this->values);
$this->action = array_shift($this->values);
share|improve this answer
    
I want to keep the array, so popping stuff out isn't really what I want, but thanks anyway. –  grjj3 Jun 22 '12 at 0:09
    
@msgdie Have added a little note about that. –  Corbin Jun 22 '12 at 0:12
    
+1 for using the copy to your advantage –  mseancole Jun 22 '12 at 4:57
    
It looks great to me. Thank you very much sir. I appreciate all the other answers as well. –  grjj3 Jun 22 '12 at 10:56
up vote 7 down vote

There seems to be some code repetition. Perhaps this could be improved like this:

if ($url_chunks_num >= 1)
   $this->controller = $this->pageData[0];
if ($url_chunks_num >= 2)
   $this->action = $this->pageData[1];
if ($url_chunks_num > 2)
   $this->values = array_slice($his->pageData, 2);

Or

if ($url_chunks_num >= 1) {
   $this->controller = $this->pageData[0];
   if ($url_chunks_num >= 2)
      $this->action = $this->pageData[1];
   if ($url_chunks_num > 2)
      $this->values = array_slice($his->pageData, 2);
}
share|improve this answer
up vote 2 down vote

this should look better.

if ($url_chunks_num>0)
    $this->controller = $this->pageData[0];
if($url_chunks_num > 1)
    $this->action = $this->pageData[1];
if($url_chunks_num > 2) {
    $this->values = array_slice($this->pageData, 2);

but also using a switch statement should be nicer... i do not know if it works, should be tested...

switch ($url_chunks_num) {
    default:
        $this->values = array_slice($this->pageData, 2);
    case 2:
        $this->action = $this->pageData[1];
    case 1:
        $this->controller = $this->pageData[0];
    case 0:
        break;
}

as you do not use breaks in cases it shold work like it is in your structure.

last exit before bridge : you should test it esp. for syntax errors.

share|improve this answer
1  
The switch syntax is correct, as is the logic (I believe). I must say though, I always avoid switch statements that fall through, and the way default is structured irks me a little. Definitely creative though! +1 –  Corbin Jun 22 '12 at 7:50
    
well using if's in this piece of code is just bad in terms of optimization... and yes i know this is just some web application, and yes i am obsessive ;) –  kubilay.kupeli Jun 22 '12 at 10:04
    
I would be surprised if there's even a 1 us difference (probably even less than that). –  Corbin Jun 22 '12 at 20:21
up vote 1 down vote

I'd do it just how Corbin suggested, but the above is still valid. You could also check out list() and, if $pageData were associative with keys of the same title you could use variable variables. Disclaimer: I do not advise the use of variable variables, merely pointing it out for completeness.

share|improve this answer
1  
this would behave differently from the original code, for example if $url_chunks_num == 0. In your code the line $this->controller = $this->pageData[0]; will be executed, while in the original it will not. –  Nadir Sampaoli Jun 22 '12 at 6:09
    
@nadirs: You are quite right, can't believe I missed that. I'll remove the code snippet, as I can't make that look any better with the added constraint. However, I'll leave the answer up for the rest is still relevant. –  mseancole Jun 22 '12 at 13:16
    
List requires you to know that the elements exist. And variable variables would have the same problem. -1 isn't me though. –  Corbin Jun 22 '12 at 20:19
    
Its actually -2. However, the issue with list can be fixed by checking its length and appending elements onto it so that it has the right default amount, even if those are empty. I'll probably just end up deleting this post as it appears that it is not going over well -.- –  mseancole Jun 22 '12 at 21:27

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.