Mehrpadin [mehrpadin]

/**
* DD_belatedPNG: Adds IE6 support: PNG images for CSS background-image and HTML <IMG/>.
* Author: Drew Diller
* Email: drew.diller@gmail.com
* URL: http://www.dillerdesign.com/experiment/DD_belatedPNG/
* Version: 0.0.7a
* Licensed under the MIT License: http://dillerdesign.com/experiment/DD_belatedPNG/#license

Files available from third-party sites should not be included in Drupal.org CVS.
In Drupal.org CVS the only accepted files are the ones licensed under GPL license v2+; compatible licenses are not accepted.

Oh didn't know that, ok removed, so what to do now?

Upload the modified theme in a new comment.

		<?php if($bottom_left || $bottom_right) { 

function numreg_top($top_a, $top_b, $top_c, $top_d, $top_e) {
	$n = 0;
	if ($top_a) { $n++; }
	if ($top_b) { $n++; }
	if ($top_c) { $n++; }
	if ($top_d) { $n++; }
	if ($top_e) { $n++; }
	print $n;
}

kiamlaluno, can you give any more actionable feedback? I'd like to see this feedback clarified both to help Mehrpadin and to improve my own understanding of the standards that apply to CVS applications, so I might help review them.

The alternative syntax, not mentioned in the coding standards on control structures, apparently refers to this from php.net, i.e. using endif; instead of curly braces. Beyond not mentioning alternative syntax, the coding standards say "You are strongly encouraged to always use curly braces even in situations where they are technically optional," which seems to actually suggest not using alternative syntax. Is this an actual coding standard documented somewhere else or just a personal preference?

And "Isn't there a more optimized way to write that code?" seems pretty vague. Obviously you think there is something to improve, but since you didn't criticize anything specific, Mehrpadin (and anyone else using this feedback to improve their own coding) can only guess at what needs to change.

@sreynen: Thanks for pointing out I was not clear in what I reported; I will explain myself better.

The alternative syntax is the one normally used on themes, especially when the template needs to conditionally add HTML tags.
Code like the following (the fragment has been taken from a Drupal core theme)

      <?php if ($left): ?>
        <div id="sidebar-left" class="sidebar">
          <?php if ($search_box): ?><div class="block block-theme"><?php print $search_box ?></div><?php endif; ?>
          <?php print $left ?>
        </div>
      <?php endif; ?> 

is more readable than

      <?php if ($left) { ?>
        <div id="sidebar-left" class="sidebar">
          <?php if ($search_box) { ?><div class="block block-theme"><?php print $search_box ?></div><?php } ?>
          <?php print $left ?>
        </div>
      <?php } ?> 

It is more readable because it more visible where a conditional statement ends, and it is more readable because it is documented which conditional statement ends.
This is true where fragments of PHP code are mixed with HTML fragments; it is not true for modules, where the content of a module file is PHP code.

function numreg_top($top_a, $top_b, $top_c, $top_d, $top_e) {
    $n = 0;
    if ($top_a) { $n++; }
    if ($top_b) { $n++; }
    if ($top_c) { $n++; }
    if ($top_d) { $n++; }
    if ($top_e) { $n++; }
    print $n;
}

Considering that the function is called from the following code

		<?php if($top_a || $top_b || $top_c || $top_d || $top_e) { ?>
		<div style="clear:both"></div>
		<div id="top" class="in<?php print numreg_top($top_a, $top_b, $top_c, $top_d, $top_e); ?>">

the function could be replaced from the following code (I write it in the context where it is called)

		<?php if($top_a || $top_b || $top_c || $top_d || $top_e) { ?>
		<div style="clear:both"></div>
		<div id="top" class="in<?php print (bool) $top_a + (bool) $top_b + (bool) $top_c + (bool) $top_d + (bool) $top_e; ?>">

Effectively there is something wrong in the code; the function is printing the result, and so does the code that is invoking the function.