HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Column selection and URL modification

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
selectioncolumnmodificationandurl

Problem

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?


$('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));
    });
});


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

EDIT:


    {% if image is defined %}
    Bilder verwalten
    Bild betrachten
    Bild hinzufügen
    Bild bearbeiten
    Bild löschen
    {% else %}
    Bilder verwalten
    Bild betrachten
    Bild hinzufügen
    Bild bearbeiten
    Bild löschen
    {% endif %}


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)

Solution


$(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);
    });
});


Here I am assuming your toolbar is outside of your table.
About the changes:

  • $(function () { lets you move this script tag elsewhere in your document (perhaps to an external script)



  • By caching $lastSelected, fewer DOM traversals need to happen.



  • Knowing $lastSelected enables moving the $.fn.each call out of the tr click event by setting name inside of it.



  • 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).



  • Instead of modifying the a tags we can just set window.location since we are now in the event where the user has clicked.



  • 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).

Code Snippets

<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>

Context

StackExchange Code Review Q#11309, answer score: 2

Revisions (0)

No revisions yet.