1
\$\begingroup\$

I usually find the .js files to be quite messy and the truth is that mine are even worse than the average as, being soft, I'm not the sharpest tool neither at jQuery nor JavaScript.

I would really appreciate if someone could give me a heads up about improving the readability (and point me bad practices) of this code. I would try to have them into account for other .js files.

$(function () {
    $('form').on("click", "div.toggler", function () {
        $(this).next().slideToggle(300);
    });

$('a.edit').click(function (e) {        
    var clickedElement = $(this).parent();
    var url = $('#editForm').data('amp-url');

    $.get(url, { id: parseInt($(this).attr('id')) }, function (result) {
        $("#editForm").html(result);

        // Display edit form just below the "item" clicked
        if ($("#editForm").is(":visible")) {
            $('#editForm').slideToggle(300, function() {
                $("#editForm").appendTo(clickedElement);
                $('#editForm').slideToggle(300);
            });
        } else {
            $("#editForm").appendTo(clickedElement);
            $('#editForm').slideToggle(300);
        }
    }, "html");
    e.preventDefault();
});

// Hide edit form when click outside it 
$(document).mouseup(function (e) {
    if ($('#editForm').is(":visible")) {
        var container = $("#editForm");

        if (container.has(e.target).length === 0) {
            container.slideUp(300);
        }
    }
});

(function (a) {

    jQuery.fn.screencenter = function () {
        this.css("position", "absolute");
        this.css("top", (($(window).height() - this.outerHeight()) / 2) + $(window).scrollTop() + "px");
        this.css("left", (($(window).width() - this.outerWidth()) / 2) + $(window).scrollLeft() + "px");
        return this;
    };
})(jQuery);
});

By the way, I have seen here that deferred objects seems cleaner (apart of other considerations). I couldn't make it work, though.

\$\endgroup\$
  • 1
    \$\begingroup\$ How do you organize your code in other languages? Don't you put the code in semantically named files? \$\endgroup\$ – Florian Margaine Apr 26 '13 at 12:13
  • \$\begingroup\$ Yes, this is in a separate file with the name of controller (which doesn't have many views, just a main one and a partial for the moment): Documents.js \$\endgroup\$ – mitomed Apr 26 '13 at 12:42
  • 1
    \$\begingroup\$ You could start by not leaning on jQuery to find that "editForm" element a zillion times over the course of your script. $('#editForm') is ugly as all get out. editForm is not. Write var editForm = $('#editForm') once (or better yet, a crystal clear document.getElementById('editForm')), use editForm from there on out, your code is looking cleaner and running faster already. $(this).attr('id') is slow and ugly, this.id is not, and so on... rinse and repeat. \$\endgroup\$ – Dagg Apr 26 '13 at 13:25
  • \$\begingroup\$ Can't vote up as in stack overflow both of your comments. Agreed that is a big improvement Dagg, but I wonder as I need to wrap these DOM object in a jquery one it's better at most do something like var editForm = $(document.GetElementById('editForm')) almost top of the file, right? \$\endgroup\$ – mitomed Apr 26 '13 at 14:02
  • \$\begingroup\$ Voting up turned out to be a privilege I just got \$\endgroup\$ – mitomed Apr 28 '13 at 7:49
1
\$\begingroup\$

As Dagg said in his comment, try to cache things as much as possible - I haven't tested this code but you could do something like this:

(function ($, window) {
    // Do stuff when the DOM is ready.
    $(function () {

        $('form').on("click", "div.toggler", function () {
            $(this).next().slideToggle(300);
        });

        $('a.edit').click(function (e) {
            var clickedElement = $(this).parent(),
                editForm = $('#editForm'), // cache the edit form element.
                url = editForm.data('amp-url');

            $.get(url, {
                id: parseInt(this.id, 10) // Always specify the radix parameter
            }, function (result) {
                editForm.html(result);

                // Display edit form just below the "item" clicked
                if (editForm.is(":visible")) {
                    editForm.slideToggle(300, function () {
                        editForm.appendTo(clickedElement);
                        editForm.slideToggle(300);
                    });
                } else {
                    editForm.appendTo(clickedElement);
                    editForm.slideToggle(300);
                }
            },
                "html");
            e.preventDefault();
        });

        // Hide edit form when click outside it 
        $(document).mouseup(function (e) {
            var container = $("#editForm");
            if (container.is(":visible")) {
                if (container.has(e.target).length === 0) {
                    container.slideUp(300);
                }
            }
        });
    });

    // Add the screen center function to jQuery.
    $.fn.screencenter = function () {
        var w = $(window);
        this.css("position", "absolute");
        this.css("top", ((w.height() - this.outerHeight()) / 2) + w.scrollTop() + "px");
        this.css("left", ((w.width() - this.outerWidth()) / 2) + w.scrollLeft() + "px");
        return this;
    };
}(jQuery, window));

This avoids having to hit the DOM each time, for example, you had $('window') four times... It's much better to get it once and store it in a variable.

I've also wrapped everything in an IIFE (Immediately Invoked Function Expression).

I'm not sure what you were trying to do here:

(function (a) {
    // code omitted.
})(jQuery);

But effectively you are passing the jQuery object into a function aliased as a. Then you are not using a but the jQuery object anyway - a bit pointless.

Edit: Regarding the deferred object question:

var idToSend = 10;
var request = $.get(url, {
    id:  idToSend
});

// Assign handlers for events (can also chain):
request.done(function (result) {
    // Code for success.  
}).fail(function (result) {
    // Code for fail
});
\$\endgroup\$
  • \$\begingroup\$ Thanks RobH, big improvement. As a minor point I declared editForm (now editContainer) inside the IIFE so I can access it both in the ajax call and the function for hiding the form. After almost two years working as C# developer I don't know how to deal properly with .js and I find that even much more senior people than me don't neither, in my environment at least I feel we're just patching things via solutions posted in the web. I'm trying to spend time on javascript and its libraries, hopefully I will be able to structure my knowledge on that. Will try now the deferred object stuff. \$\endgroup\$ – mitomed Apr 27 '13 at 11:57
  • \$\begingroup\$ @mitomed Yeah I know what you mean. I'm predominantly a C# developer (in SharePoint actually) but really like JavaScript. I recommend you ready JavaScript: The Good Parts by Douglas Crockford. It's an amazing book and gives a great insight into how good JavaScript actually is! \$\endgroup\$ – RobH Apr 29 '13 at 10:10

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.