current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

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 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" src="{{ asset('bundles/acme/js/jquery.js'></script>
<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(){
        var oldUrl = $(this).attr('href');
        var newUrl = oldUrl.replace('var', name);
        $(this).attr('href', newUrl);
    });
});
</script>
share|improve this question

migrated from stackoverflow.com Apr 30 '12 at 12:29

This question came from our site for professional and enthusiast programmers.
    
fiddle please.. –  gdoron Apr 29 '12 at 13:53
1  
Your code is fine, only thing i would change is giving the table an ID, so it dosent bind all tables, and it find the element faster. And combine oldUrl and newUrl into one line, no reason to make the oldurl since you don't use it as an var. –  Marco Johannesen Apr 29 '12 at 14:27

1 Answer 1

up vote 2 down vote

I do not like too much this:

var name = $(this).find('td:nth-child(2)').html();

I would generally use html5 data attribute on the tr row to pass the id.

example:

<table>
   <tr data-id="1"><td>bla</td><td>bla</td><td>bla</td></tr>
   <tr data-id="2"><td>bla</td><td>bla</td><td>bla</td></tr>
</table>

var selectedId = $(this).data("id");
share|improve this answer
    
Hi, the problem is that I don't directly use the id as identificator. I mostly use a slug which is in some <td>-block. But good idea for normal identification! –  user1246987 Apr 29 '12 at 16:38

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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