Assuming you have a code such as this:
foreach ($products as $product) {
process(product);
if(somecondition){
doSomething(product);
}
}
How bad would you consider this form of refactoring?
foreach ($products as $product) {
process(product);
}
foreach ($products as $product) {
if(somecondition){
doSomething(product);
}
}
The reason I like it is because you can extract both things in separate methods, and I do not feel that the performance is hurt that bad...
Let's separate this to two questions,
First, code optimization vs readable code, I don't see how the second one more readable than the first one. For you maybe, but for people new to the project, they will sure ask why do you need two of the same loop?
Second, "I do not feel that the performance is hurt that bad", that's depends, if you have less than 100 products maybe (but still take performance hit, not noticeable though). But as your products grow, the second code will always take twice the time as the first one.
Without further context, what you did was
What you really should do is KISS and DRY. Make the code easy to read and understand, optimize it for computation time and remove complex structures, such as loops. Just as an example: Loops in your code have to be interpreted, while using built-in methods, which loop over iterable structures, run on the hardware directly.
So, taking your first code snippet, an optimization would look like this:
function processProducts($product, $key) {
// you might want to put your process code in this function
// or split it up in some other way
process($product);
if (somecondition) {
doSomething($product);
}
}
array_walk($products, 'processProducts');
Depending on your PHP architecture and real benchmarks, that approach might be disruptive, so you have to decide if it is worth implementing.
If you choose your second method you're doing the calculation twice. i.e. iterating through each product instead of just once. It approximately doubles the computation time.
Personally I prefer the first. I'd just add a comment or two on the methods being used.
To add to what Gergely Polonkai said, I would also add that you should probably start by questioning why you want to separate things into separate methods anyway. Sometimes it makes sense to, but more often it's actually more of a burden and you are making your code harder to follow at higher level. Sometimes you really shouldn't separate things out - business logic for example becomes difficult to manage if it's spread too thinly and dotted around in different places.
The performance drawback depends on how long your $products variable is. If it contains a lot of items, it may affect performance, especially if this piece of code runs a lot.
Also for me, it’t pretty WET (We Enjoy Typing, as of the opposite of DRY, Don’t Repeat Yourself). Unless you provide me any technical reason why you did this, I won’t approve this.
Giovanni Benussi Paredes
What about this:
foreach($products as $product) { process(product); } if (somecondition) { foreach($products as $product) { doSomething(product); } }This way you instantly know that if
someconditionis true,doSomethingwill be executed on each product (I think that this is subtle, but useful) and also you don't have to ask forsomeconditionevery time in theforeachloop.