3
\$\begingroup\$

I have an HTML table representing a database table and a sidebar which holds different anchors ready to perform CRUD operations on click.

Because the hrefs in the anchors aren't in general valid for all columns I have written a script which automatically inserts the id of a selected column into the hrefs:

/localhost/image/var/edit => /localhost/image/4/edit

The code I have written works but it isn't very clean and smart :( Could somebody take a look and say where I have space of improvement?

<script type="text/javascript">
$('table tr').click(function(){
    $('.selected').removeClass('selected');
    $(this).addClass('selected');
    var name = $(this).find('td:nth-child(2)').html();

    $('.image_bar ul li a').each(function(){
        $(this).attr('href', $(this).attr('href').replace('var', name));
    });
});
</script>

How could I improve the td:nth-child(2) selector and the long line modifying the link?

EDIT:

<aside class="left image_bar">
<ul>
    {% if image is defined %}
    <li><a href="{{ path('AcmeCoreBundle_Image_index')}}">Bilder verwalten</a></li>
    <li><a href="{{ path('AcmeCoreBundle_Image_show', { name: image.name }) }}">Bild betrachten</a></li>
    <li><a href="{{ path('AcmeCoreBundle_Image_new') }}">Bild hinzufügen</a></li>
    <li><a href="{{ path('AcmeCoreBundle_Image_edit', { name: image.name }) }}">Bild bearbeiten</a></li>
    <li><a href="{{ path('AcmeCoreBundle_Image_delete', { name: image.name }) }}">Bild löschen</a></li>
    {% else %}
    <li><a href="{{ path('AcmeCoreBundle_Image_index')}}">Bilder verwalten</a></li>
    <li><a href="{{ path('AcmeCoreBundle_Image_show', { name: 'var' }) }}">Bild betrachten</a></li>
    <li><a href="{{ path('AcmeCoreBundle_Image_new') }}">Bild hinzufügen</a></li>
    <li><a href="{{ path('AcmeCoreBundle_Image_edit', { name: 'var' }) }}">Bild bearbeiten</a></li>
    <li><a href="{{ path('AcmeCoreBundle_Image_delete', { name: 'var' }) }}">Bild löschen</a></li>
    {% endif %}
</ul>
</aside>

As you see when having an available singe object (while editing, showing, deleting) the server side renders the right id/name into the href. Under circumstances when I only have a collection of objects or none object available, jquery has to do the rendering of the right urls. Also some URLs don't require an id (new, index)

\$\endgroup\$
  • \$\begingroup\$ $('.image_bar ul li a') selects all elements from document, not from the selected row. Are you sure that it is correct? \$\endgroup\$ – Michael Apr 29 '12 at 20:43
  • \$\begingroup\$ Yes, I have a sidebar which contains the hrefs to the action urls (new, edit, delete,...). The tables are selected above :) \$\endgroup\$ – kyogron Apr 30 '12 at 5:12
2
\$\begingroup\$
<script type="text/javascript">
$(function () {
    var $lastSelected = $([]);
    $('tr').click(function () { //tr tag is always inside a table
        $lastSelected.removeClass('selected'); //by caching this variable in the closure we don't need to traverse for it again
        $lastSelected = $(this).addClass('selected');
    });

    $('.image_bar a').click(function (e) { 
        var url = $(this).attr('href').split('var'), //both checks if var is there and prepares for the replacement
            name;
        if ($lastSelected.length === 0) {
            return url.length==1; //any urls that do not have var in them will still work
        }

        name = $lastSelected.children().eq(2).html();
        window.location = url.join(name);
    });
});
</script>

Here I am assuming your toolbar is outside of your table.

About the changes:

  1. $(function () { lets you move this script tag elsewhere in your document (perhaps to an external script)
  2. By caching $lastSelected, fewer DOM traversals need to happen.
  3. Knowing $lastSelected enables moving the $.fn.each call out of the tr click event by setting name inside of it.
  4. Since the .each call is no longer in the click event, it can be a click event of its own (no more need to iterate over it).
  5. Instead of modifying the a tags we can just set window.location since we are now in the event where the user has clicked.
  6. Finally, if a row is not selected up front, the $lastSelected variable will not provide a valid name. Thus we shouldn't allow the link to be clicked if it contains var (done by the return statement).
\$\endgroup\$
  • \$\begingroup\$ Hi, I have added the twig-template of the sidebar. So you see how I thought the second part. This is also the reason why I have to use .each(). The ideal would be if I could set a js-compatible variable (at the moment the string 'var') and jQuery only has to insert this one. This would solve many things at once: 1. The user can't request a none existing side when not selected something 2. We would spare the parsing Is there any solution which would come this idea next? Regards, Bodo \$\endgroup\$ – kyogron Apr 30 '12 at 5:27
  • \$\begingroup\$ Nice answer! What's the difference between $(function() {}); and (function() {})()? \$\endgroup\$ – Quentin Pradet Apr 30 '12 at 9:15
  • \$\begingroup\$ @Cygal: (function () {}()) is an automatically executing anonymous function. $(function () {}) is shorthand for $(document).ready(function () {}) with the benefit that $(document) doesn't need to be created. \$\endgroup\$ – Bill Barry Apr 30 '12 at 12:47
  • \$\begingroup\$ @BillBarry I knew about IIFEs but not about that shorthand. Thanks! \$\endgroup\$ – Quentin Pradet Apr 30 '12 at 12:50
  • \$\begingroup\$ @BillBarry did you read my update? Any suggestions how I could implement a better placeholder? \$\endgroup\$ – kyogron Apr 30 '12 at 18:01

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.