3
\$\begingroup\$

In my CodeIgniter project I have a Login controller that load this view:

<ul class="sub-menu">
    <li>
        <a href="javascript:;" class = "menu-item" data-target="../indoamericana/soporte/callSupport/"> Mis solicitudes </a>
    </li>
</ul>

The controller: soporte, function: callSupport

public function callSupport($index = null, $order = null){
    $session_data = $this->session->userdata('session_user');
    $data['solicitud']  = $this->helpdesk_model->getRequest($session_data['usuario'], $index, $order); 
    $data['categories'] = $this->helpdesk_model->getCategories();
    $this->load->view('modules/help_desk/supports', $data);
}

With jQuery I call the controller and render in a page content that is a <div> element:

$(".sub-menu .menu-item, .module-item").click(function(event) {
    $(".page-content").load($(this).data('target'));  
    $("title").text( $( this ).text() );
});

In the view 'modules/help_desk/supports':

<table class="table table-striped table-hover" id="admin-support" data-target="soporte/callSupport/"> 
<thead>
<tr>
    <th>Estado <a href="javascript:;"><i class="icon-sort" data-sort="status"></i></a></th>
    <th>Prioridad <a href="javascript:;"><i class="icon-sort" data-sort="priority"></i></a></th>
    <th>Responsable <a href="javascript:;"><i class="icon-sort" data-sort="id_responsible"></i></a></th>
</tr>
</thead>
<tbody>
    <?php foreach ($solicitud as $key => $value) { ?>
    <tr>
        <td><span class="label label-sm <?= $value['label']?> "><?= ucfirst($value['status']) ?></span></td>
        <td><span class="badge <?= $value['class']?> "><?= $value['priority'] ?></span></td>
        <td><?= $value['name'].' '.$value['last_name'] ?></td>
    </tr>
    <?php  } ?>
</tbody>
</table> 

As you can see, in the first <td> a put an anchor tag calls another controller with jQuery similar than before. Please be critical, very severe with me.

\$\endgroup\$
  • \$\begingroup\$ If you are lucky, you'll never have a help desk employee called <script>alert(123)</script>. I wouldn't count on that, though. Better protect your code against cross-site scripting. \$\endgroup\$ – Roland Illig Mar 2 at 6:08
1
\$\begingroup\$

General comments

I don't see much wrong with the code. The biggest concern I might have is that PHP code is mixed in with the HTML - an alternative to this would be using a template system. CodeIgniter has the template parser class which should allow you to do this.

Please be critical, very severe with me.

Okay - I am not typically this critical but you asked for it! The next section has a couple other review points

Minor nitpicks

In the PHP code, $data is not declared before values are assigned to specific keys:

$data['solicitud']  = $this->helpdesk_model->getRequest($session_data['usuario'], $index, $order); 

I don't see a problem with this practice but the PHP documentation advises against it:

If $arr doesn't exist yet, it will be created, so this is also an alternative way to create an array. This practice is however discouraged because if $arr already contains some value (e.g. string from request variable) then this value will stay in the place and [] may actually stand for string access operator. It is always better to initialize a variable by a direct assignment.


I tried running the jQuery callback through the demo page on eslint.org. It showed this warning:

'event' is defined but never used.

For this line:

$(".sub-menu .menu-item, .module-item").click(function(event) {

While event could be used if necessary, it isn't needed.

\$\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.