2
\$\begingroup\$

When I look at this style of code I feel like there has to be a better way to write this:

<div id="by_cx">
    <!-- free text input for name -->
    <label>Name:<input type="text" id="cxByNameInput" name="cxByNameInput"/></label>
    <!-- cx state drop down -->
    <label for="cxState">State:</label>
    <select id="cxState">
        <option></option>
    <?$states = getStates();
    while($state = $states->fetch_assoc())
    {?>
        <option value="<?=$state['st']?>"><?=$state['state']?></option>
    <?}?>
    </select>
    <!-- cx status drop down -->
    <label for="cxStatus">Status</label>
    <select id="cxStatus">
        <option></option>
    <?$statusList = getCxStatusList();
    while($status = $statusList->fetch_assoc())
    {?>
        <option value="<?=$status['cxType']?>"><?=$status['cxTypeDescription']?></option>
    <?}?>
    </select>
</div>

I'd like to mention that keeping eclipses HTML code coloring would be a huge bonus...

\$\endgroup\$
5
\$\begingroup\$

Code separation

What you have there is everything mixed together - just by moving the php logic to the top of the file, makes it easier to read. With some minor reformatting it becomes easier to read/maintain:

<?php
$states = getStates();
$statusList = getCxStatusList();
?>  
<div id="by_cx">
    <!-- free text input for name -->
    <label>Name:<input type="text" id="cxByNameInput" name="cxByNameInput"/></label>
    <!-- cx state drop down -->
    <label for="cxState">State:</label>
    <select id="cxState">
        <option></option>
    <?php while($state = $states->fetch_assoc()): ?>
        <option value="<?= $state['st'] ?>"><?= $state['state'] ?></option>
    <?php endwhile; ?>
    </select>
    <!-- cx status drop down -->
    <label for="cxStatus">Status</label>
    <select id="cxStatus">
        <option></option>
    <?php while($status = $statusList->fetch_assoc()): ?>
        <option value="<?= $status['cxType'] ?>"><?= $status['cxTypeDescription'] ?></option>
    <?php endwhile; ?>
    </select>
</div>

Format for readability

If you're jumping in and out of php - having lines like this: {?> make things hard to read, especially if there is some nesting in the code. Whether you choose to use curly braces or the alternate (colon) syntax is up to you, but in html code using a style which aides readability (putting the curly brace/colon on the same line as the statement it relates to) helps.

No PHP Short tags

Shorttags are often considered a bad practice, and as such should be avoided unless the code is your own, and you control where it's going to be used. Note that you can use <?= with PHP 5.4 irrespective of the shorttags setting.

Consistent whitespace

The following:

<? foo(); ?>

Is easier to read than

<?foo();?>

Especially where it appears inline somewhere - use whitespace for readability.

Sprintf

If you find that your code gets to be like this:

<foo x="<?= $x ?>" y="<?= $y ?>"  z="<?= $z ?>"> ...

Then it's probably easier to read and maintain using:

<?php echo sprintf('<foo x="%s" y="%s" z="%s">...', $x, $y, $z); ?>
\$\endgroup\$
  • \$\begingroup\$ Interesting, so it would seem that other than the code separation you mentioned, there isn't much to improve on being that a lot of the other issues are just personal preference things ie. readablity. I had wondered if the while loop should actually be removed from inside the <select> tag but it would seem not. Thank you \$\endgroup\$ – Bill.Caffery Oct 29 '12 at 20:38
  • 2
    \$\begingroup\$ I wouldn't say that ;). There are changes outside the scope of your question as posed. View-like code should be as dumb as possible - the while loops should really be a foreach iterating over an array (allows for caching and or testing without the use of a db), "complex" html should be rendered with a helper function. These are two examples from the code in the question - look at any framework for something to compare to. \$\endgroup\$ – AD7six Oct 29 '12 at 21:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.