3
\$\begingroup\$

I have taken a stab at creating my first jQuery script from scratch. I use this to post an update to a Django application that allows a user to "follow" another user. I am sure that there are better ways to do this and I appreciate any help that anyone can give.

<script type="text/javascript">
jQuery(function() {
    jQuery('.follow').click(function(event) {
        event.preventDefault();
        var url = jQuery(this).attr("href");
        var followtype = jQuery(this).text();
        var user = jQuery(".follow").attr("href").match('/relationships/[a-z]+/([a-z]+)/following/')[1];
        var postdata = {
            'csrfmiddlewaretoken': '{{ csrf_token }}'
        }
        jQuery.post(url, postdata, function(json){
            if(json.result == '1') {
                if(followtype == 'Follow') {
                    jQuery('.follow').text('Unfollow');
                    jQuery('.follow').attr("href", '/relationships/remove/'+user+'/following/');
                } else {
                    jQuery('.follow').text('Follow');
                    jQuery('.follow').attr("href", '/relationships/add/'+user+'/following/');
                }
            }
        })
    });
});
</script>
\$\endgroup\$
2
\$\begingroup\$

This is how I'd do it. There may be flaws because you haven't provided your HTML.

I assume that within the click handler, you only want to interact with the current element being clicked. The reason I say that is because you had $('.follow') a number of times in there, which would affect all .follow elements.

         //     v--- makes the shortcut available inside the .ready() callback
jQuery(function($) {
    $('.follow').click(function(event) {
        event.preventDefault();

            // assuming it's a link, no need for jQuery here
        var url = this.href;

            // Looking at the code in your callback, I assume you only have text content
        var followtype = this.innerHTML;

            // I assume you actually want just the current "follow", not all
        var user = url.match('/relationships/[a-z]+/([a-z]+)/following/');

            // Make certain there was a match before accessing the Array.
            // If there was no match, just return the function
        if( user )
            user = user[1];
        else
            return;

        var postdata = {
            'csrfmiddlewaretoken': '{{ csrf_token }}'
        };
            // Inside the $.post callback, are we still only working with the current
            //   "follow" clicked? If so, cache it here, and use it in the callback
        var this_follow = this;

        $.post(url, postdata, function(json){
            if(json.result == '1') {
                if(followtype == 'Follow') {
                     this_follow.innerHTML = 'Unfollow';
                     this_follow.pathname = '/relationships/remove/'+user+'/following/';
                } else {
                    this_follow.innerHTML = 'Follow';
                    this_follow.pathname = '/relationships/add/'+user+'/following/';
                }
            }
        })
    });
});

As you can see, I tend to eliminate jQuery when convenient.

I should note that using .innerHTML in the way I have is cheating a bit. It's an HTML parser really, but it's convenient for changing small chunks of text quickly.

To do it properly, you'd select the child text node of the element, and change its nodeValue.

this_follow.firstChild.nodeValue = 'Unfollow';
\$\endgroup\$
2
\$\begingroup\$

Two generic notes:

  1. I'd invert the condition:

    jQuery.post(url, postdata, function(json){
        if(json.result != '1') {
            return;
        }
        if(followtype == 'Follow') {
            jQuery('.follow').text('Unfollow');
            jQuery('.follow').attr("href", '/relationships/remove/'+user+'/following/');
        } else {
            jQuery('.follow').text('Follow');
            jQuery('.follow').attr("href", '/relationships/add/'+user+'/following/');
        }
    }
    

    It makes the code flatten and more readable.

    References: Replace Nested Conditional with Guard Clauses in Refactoring: Improving the Design of Existing Code; Flattening Arrow Code

  2. Lots of duplication could be eliminated with local variables:

        var text;
        var command;
        if (followtype == 'Follow') {
            text = 'Unfollow';
            command = 'remove';
        } else {
            text = 'Follow';
            command = 'add';
        }
        var follow = jQuery('.follow');
        follow.text(text);
        follow.attr("href", '/relationships/' + command + '/' + user + '/following/');
    
\$\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.