I'm a total PHP newb, so there is probably a better way of outputting a row's class based on different variables.
Is this bad, and if it is why is it?
if ($variable1 > 0 && $variable2 != 2) {
echo "<tr class='variable1'>";
}
elseif ($variable2==2)
{
echo "<tr class='variable2'>";
}
else {
echo "<tr>";
}
I would write it like this:
$class_name = '';
if (2 == $variable2)
{
$class_name = 'variable2';
}
elseif ($variable1 > 0)
{
$class_name = 'variable1';
}
echo '<tr class="' , $class_name , '">';
notice that on the equality test I use the constant before the variable.. this is a habit so that if I miss one equal it would result in an error that I would be forced to correct it instead of a "strange" behavior
another thing is that I give the initial value to $class_name and that will be the default one
also I use apostrophes instead of quotation marks because it is faster (because the way you did it php will parse the string for variables)
and the last thing is that I echo multiple string.. that's also faster than concatenating 3 strings (
echo '<tr class="' , $class_name , '">';
and not echo
'<tr class="' . $class_name . '">';
)
Well you could start with
if ($variable2==2)
which would safe you the $variable2 != 2 if you continue with
elseif ($variable1 > 0)
but generally I see no mistake. Of course this depends on what else you want to do.
You could put the if ($variable2==2)
first and then you wouldn't need to negate it on the other if statement. Like this:
if ($variable2==2){
echo '<tr class="variable1">';
}elseif ($variable1 > 0) {
echo '<tr class="variable2">';
}else {
echo '<tr>';
}
Looks okay to me though, but it depends on the rest of the code really, you might be able to change the code slightly to make it easier to read but it looks pretty simple so, if it works, i'd keep it how it is.
The php code is syntactically correct, although the indentation seems to be messed up. The outputted HTML code is invalid though, since attributes values must be enclosed in double, not single quotes.
I'd also suggest a slight reordering:
if ($variable2 == 2) {
echo '<tr class="variable2">';
} elseif ($variable1 > 0) {
echo '<tr class="variable1">';
} else {
echo '<tr>';
}
echo ( ( $variable1 > 0 ) && ( $variable2 != 2 ) )
? "<tr class='variable1'>"
: ( $variable2 == 2 )
? "<tr class='variable2'>"
: "<tr>";
2 rules for you to learn how to program:
.1. DRY: Don't Repeat yourself.
if you have your tr tag typed 3 times, you can say for sure that's wrong.
.2. Separate business logic from presentation logic. Use templates for output. Prepare your data first and start output only if everything is ready.
So, first you have to define your variables
if ($variable2 == 2){
$class = "variable1";
}elseif ($variable1 > 0) {
$class = "variable2";
}else {
$class = "";
}
next you have to include a template and write native HTML in it, with as little PHP as possible:
<tr class="<?=$class?>">
Well, that is a really unfortunate approach at many levels, but I can tell you that even I started writing code the way you describe, only that was over 12 years ago.
I can also tell you that eventually mixing php with markup in the same files will come to bite you in the rear, and sooner than you think. As with most things in life, getting it done faster and worrying about the rest later is a bad approch when developing in PHP.
A few pointers:
A couple of links: