3

In some code I'm working on I have noticed that several variables are being accessed from outside of foreach loops.

This is seen in codeigniter view files of this particular app.

For example, a view file:

<?php
foreach ($items as $row):
endforeach;

// HTML / PHP code...

 <td>
     <?php echo form_checkbox('option_1','1', FALSE); ?>
     <?php echo form_hidden('weight_unit', $row->weight_unit); ?>
 </td>
// etc...

This works (ie, no errors) but I wonder if this would be considered a bad practice and if so, why? (scope, etc)

Does anyone have an opinion on this and should variables only be called inside their corresponding loops?

Another issue I've noticed is if a variable is required in several parts of a view file: should I refactor to have multiple loops or should there be a single foreach / endforeach and the begin / end of the file.

Any suggestions are much appreciated. Thanks.

Improve this question

4 Answers 4

Reset to default
3

The only reason I could think of doing that is to seek to the end of the array so you can store its last member in a variable.

You can do this clearer with end().

$row = end($items);
Improve this answer
Sign up to request clarification or add additional context in comments.

4 Comments

Completely agreed. If the body of the loop is empty, it's definitely a waste to do the iteration.
that's an interesting point - there's one file in which an empty foreach / endforeach is invoked at the top, and several variables (not only the last array member) are called from that empty loop at the middle/bottom of the file -- and it works. does this make any sense?
@torr, what do you mean by "variables"? What do they look like? The iteration only gives you one new one: $row which is the last item.
sorry I meant other members of the array -- just double checked the code and you're right, this method will only call the last member -- I got confused by the code because there was another empty foreach loop that assigned the other array members to variables that were accessed elsewhere
2

$row will be the last item in the array, or unset, when it reaches your other code.

Is that want you want? If so, then it's not bad practice, per se. But you should at least document that the behavior is intended, because it's not intuitive.

Better practice is something like:

foreach ($foo as $bar) 
{
  // do something
}
$last_bar = isset($bar) ? $bar : null;

There it's obvious that you mean to do something with $last_bar.

Another issue I've noticed is if a variable is required in several parts of a view file

If you must iterate over your loop in different places in your view, then that's okay.

But if you just need a certain piece of information deep inside some array, then you should stick it into an easily accessible variable and use that instead.

Improve this answer

Comments

2

This is a bad practice. If the recordset is more than one item long then you are looping through all the records (even though you do nothing in the loop) and then just using the last record. I dont use CI but if this is a recordset object there ought to be someway to access the first/last record or any other index position in the RS. You should use those if youre after a particular record. If its just an array you could use array_pop if you dont need to keep the array intact, or end if you do.

Additionally, in the same context of a lengthy set if its not really the last record your after this could have unforseen consequences if the recordset length changes.

Improve this answer

2 Comments

I agree this shouldn't be done - see my comment to @alex above -- it looks like other variables, not only the last member, are being accessed (successfully) from the empty loop in this code
please see above - there was some confusion about how the code was set up - this (bad) practice will only provide the last member as you said
1

I'd say this is really bad practice - what happens if later (in a few weeks) you come across the same piece of code and you need another foreach on totally different record types between the actual foreach and your table? - your $row variable will then have a totally different meaning - this piece of code is very susceptible to side effects when more code is added.

Also, there is a high possibility that in the future this behavior will not be supported by PHP anymore (this is just an assumption of mine, because, as you mentioned, the $row variable is out of scope where you are using it).

As some general principles:

  • use as few as possible global variables - they are hard to track, maintain and assure correct values
  • minimize the scope of the variables as much as possible and use them only in very clear scopes
  • avoid marginal use cases and so called features that are totally unnatural in other languages
  • last, but not least, avoid the principle if code was hard to write it should be hard to read - this principle won't secure you the job, only the hate of your coworkers
Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.