1

I have form for my recipe app that inserts ingredients into a db. If nothing has been submitted yet, nutritional values display '0'. Otherwise they update to the post values.

My structure for the nutritional display is:

Calories:
  <?php if ($ingredientArray[calorieKey] >= 1)
  echo $ingredientArray[calorieKey];
    else echo 0; ?><br />
Protein:
  <?php if ($ingredientsArray[proteinKey] >= 1)
  echo $ingredientArray[proteinKey];
    else echo 0; ?><br />

...and continues for about 20 items. The question is:

would it be more efficient to refactor the code using just 1 if/else statement to display 2 different forms (dynamic and static), or is it better as is?

hakre
  • 193,403
  • 52
  • 435
  • 836
elev8
  • 25
  • 3

4 Answers4

3

Efficiency here doesn't matter at all. Unless you have literally millions of these duplicate if-statements, the processing time is effectively zero, compared to all of the other things that cause your page to take time to load (like fetching data from the database, sending the output to the client, and the client rendering it in their browser, etc).

So you should focus on making your code as readable and maintainable as possible.

There are times when you should sacrifice readability for better performance... but until you actually experience performance problems, you're still miles away from that point.

Jonathan Hall
  • 75,165
  • 16
  • 143
  • 189
  • my question is more for general knowledge. i just used the above as an example. and since it takes just as much time to write efficient code as it does crap code, i'd rather learn how to write good code while i have to the time to do so. – elev8 Oct 28 '11 at 07:28
  • @elev8: It's often faster to write good code than to write crap code. :) Even so, focus on code readability and maintainability. Don't give into premature optimization. – Jonathan Hall Oct 28 '11 at 07:31
  • @elev8: That's not my point :) Although it probably could be improved. Several other answers here have offered some suggestions for improving readability. I especially like @PeterSzymkowski's use of the `? :` operator instead of `if .. else`. – Jonathan Hall Oct 28 '11 at 07:37
2

What you need is just a loop;

$recipes = array('caloriesKey' => 'Calories', 'proteinKey' => 'Protein');
foreach($recipes as $key => $value) {
    echo $value . ($ingredientArray[$key] >= 1 ? $ingredientArray[$key] : 0) . '<br />';
}
xdazz
  • 158,678
  • 38
  • 247
  • 274
1
<?php echo $ingredientArray[calorieKey] >= 1 ? $ingredientArray[calorieKey] : 0 ?>

Looks better.

Stop worrying about it, and take care of the code purity.

Peter
  • 16,453
  • 8
  • 51
  • 77
0

How about a block of code that will create another variable, let's call it $displayArray, which will hold the actual values you want to print?

Then, you can just loop through $displayArray when you're constructing your view.

This may not seem like a big difference but it makes it much easier to figure out what's going on for the next guy, and it lets you change, or even swap out the way you're displaying the same set of information much more easily.