0
\$\begingroup\$

Below is code i used to call Api and display the "Status" results in web page.

enter image description here

my senior told me that code is completely mess and need to improve so that it should be easy to understand for others. but i am not getting how i can refactor below code ? is mixing php and html code is bad ? i really need to mix those html and php to get the results that i needed .please help me for this....

<p><button type= "button" class="call">Show Status</button></p>

    <table class="tbl-qa" border="1">
        <thead>
            <tr>    
               <th class="table-header"></th>     
               <th class="table-header">ORDERID</th>                                                      
               <th class="table-header">Status</th>                    
            </tr>
        </thead>

       <tbody id="table-body">
<?php
$tabindex = 1;

if (!empty($orderrecords))
    {
    foreach($orderrecords as $k => $v)
        { ?>

        <?php
        $hide = '';
        $data['username'] = 'admin';
        $data['password'] = 'admin123';     

        $url = 'https://plapi.ecomexpress.in/track_me/api/mawbd/?awb=awbnumber&order=' . $orderrecords[$k]["order_id"] . '&username=admin&password=admin123';
        $ch = curl_init();
        // some curl code
        $res = explode("\n", $output);
        if (!isset($res[13]))
            {
            $res[13] = null;
            }

        $status = $res[13];
?>

<tr class="table-row" id="table-row-<?php echo $orderrecords[$k]["id"]; ?>" tabindex="<?php echo $tabindex; ?>">

<td><input onclick="assignorderids('<?php echo $orderrecords[$k]["order_id"]; ?>')" type="checkbox" name="assigneeid" id="assigneeid-<?php echo $orderrecords[$k]["order_id"]; ?>" value="<?php echo $orderrecords[$k]["order_id"]; ?>"></td>

<td><?php echo $orderrecords[$k]["order_id"]; ?></td>             
<td><?php echo $status; ?></td>

</tr>


 <?php
 $tabindex++;
 }
    } ?>

    </tbody>
    </table>
\$\endgroup\$
  • 1
    \$\begingroup\$ Why don't you ask senior what is wrong? \$\endgroup\$ – Toto Feb 22 '18 at 15:03
  • \$\begingroup\$ @Toto he is not permanent employee working in our company , he usually visits some time in a week and check code of all beginner developers and tell us to improve, can you please help me for this..... \$\endgroup\$ – vickey colors Feb 22 '18 at 15:04
1
\$\begingroup\$

I can totally understand the frustration of your colleague that will need to maintain this code.

But would have been more constructive to tell that this code is considered a "mess" mainly because:

  • Logic of fetching order status is mixed in the middle of table rendering;
  • "Dirty" formatting: inconsistent indentation and trailing spaces;

The exact same thing could be better expressed like that:

<?php
function getOrderTrackingStatus($orderId) {
    $url = 'https://plapi.ecomexpress.in/track_me/api/mawbd/';
    $url.= "?awb=awbnumber&order=$orderId&username=admin&password=admin123";

    $ch = curl_init();
    // some curl code
    $res = explode("\n", $output);

    $status = isset($res[13]) ? $res[13] : null;
    return $status;
}
?>

<p>
    <button type="button" class="call">Show Status</button>
</p>

<table class="tbl-qa" border="1">
    <thead>
        <tr>
            <th class="table-header"></th>
            <th class="table-header">ORDERID</th>
            <th class="table-header">Status</th>
        </tr>
    </thead>

    <tbody id="table-body">
    <?php $tabindex = 1; ?>
    <?php foreach($orderrecords as $record): ?>

        <tr class="table-row"
            id="table-row-<?php echo $record['id']; ?>"
            tabindex="<?php echo $tabindex++; ?>">
            <td>
                <input type="checkbox"
                       name="assigneeid"
                       id="assigneeid-<?php echo $record['order_id']; ?>"
                       value="<?php echo $record['order_id']; ?>"
                       onclick="assignorderids('<?php echo $record['order_id']; ?>')">
            </td>
            <td><?php echo $orderId; ?></td>
            <td><?php echo getOrderTrackingStatus($orderId); ?></td>
        </tr>

    <?php endforeach; ?>
    </tbody>
</table>

Ideally this getOrderTrackingStatus function would be moved somewhere else, instead of living inside the same file that handles data visualisation.

Hope it helps you, and depending on your project you should consider using a templating engine as a way to force yourself to move logic code away from the views.

\$\endgroup\$

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.