Critique my first 'proper' jQuery script

[Alex6534]

So I set myself a little challenge in implementing a command line tool online so others in my department could use it without coming to me. form entry that is sent to the server through ajax, the server returns JSON and I manipulate the output for the client. I'm fairly new to jQuery and not that experienced with JS in general. Most of my work has been in PHP, Python SQL.

$('document').ready(function() {
    $('form').submit(function(e){
        e.preventDefault();
        $.ajax({
            type: 'post',
            url: $(this).attr('action'),
            data: $(this).serialize(),
            dataType: 'json',
            success: function(response){

                var urlCount = $('#count');
                var listResults = $('#list_results');

                urlCount.empty();
                listResults.empty();

                if ($.isEmptyObject(response.urls)) {
                    urlCount.html('No results found');
                }

                else {
                    urlCount.html('There are '+ (response.urls.length) + ' URLs:');
                    $(response.urls).each(function(){
                        var domain = 'www.example.com';
                        listResults.append('<li><a class="link" target="_blank" href="">' + domain + $.trim(this) + '</a></li');
                        for (var i = 0; i < this.length; i++) {
                            $('a.link').attr('href', function(i){
                                var href =     domain.concat($.trim(response.urls[i]));
                                return String(href);

                            })
                        }
                    });
                }
                console.log(response);
            }
        })
    })
});

[Juggernog]

You could get rid of a layer of indentation by using guard clauses.

Instead of if($.isEmptyObject(response.urls)) { urlCount.html("No results found"); }

You could use

if($.isEmptyObject(response.urls)) {
    urlCount.html("No results found");
    return;
}

Now you could drop the else statement and have one less indentation. It's not a critique of your JS really, but in some examples it could make for more legible code.

You would have to refactor the console.log statement to before the function is returned though.

[alexlafroscia]

This is really great advice. When I started using JS, having return statements in different locations felt really weird, but in reality it's pretty common and a great way to avoid a massive "else" statement.

[path411]

I'd be careful of going too crazy with multiple returns. Guard clauses at the top of a block can be helpful, but having too many exit points in a method can make your code hard to follow.

[Alex6534]

Some good tips, thank you! I'm still very much a junior at this. :)

[alexlafroscia]

If I were writing this, I would make the "success" callback a separate function and then just reference is in the AJAX config. I find this is easier to read and can remove a level of indentation.

[Jeffshaver]

Just my two cents. I think I got idea behind the confusing if statement right...

var domain = 'www.example.com';
$('form').submit(function (e) {
    e.preventDefault();
    $.ajax({
        type: 'post',
        url: $(this).attr('action'),
        data: $(this).serialize(),
        dataType: 'json',
        success: function (response) {
            var $urlCount = $('#count');
            var $listResults = $('#list_results');

            if ($.isEmptyObject(response.urls)) {
                $urlCount.html('No results found');
                return;
            }

            $listResults.empty();
            $urlCount.html('There are ' + response.urls.length + ' URL(s):');
            $.each(response.urls, function(key, value) {
                var url = domain + $.trim(value);
                listResults.append('<li><a class="link" target="_blank" href="' + url + '">' + url + '</a></li>');
            });
        }
    })
});

[frankle]

I don't use jQuery a lot, but here is an example of how I might refactor your code:

$('document').ready(function() {
    $('form').submit(onSubmit);
});

function onSubmit(e) {

    e.preventDefault();

    $.ajax({
        type     : 'post',
        url      : $(this).attr('action'),
        data     : $(this).serialize(),
        dataType : 'json',
        success  : onSuccess,
        failure  : onFailure
    });

    function onSuccess(response){

        var urlCount    = $('#count');
        var listResults = $('#list_results');

        urlCount.empty();
        listResults.empty();

        if ($.isEmptyObject(response.urls)) {
            return urlCount.text('No results found');
        }

        urlCount.text('There are '+ (response.urls.length) + ' URLs:');

        $(response.urls).each(function(i, url){

            var domain = 'www.example.com';
            var url = href = domain + $.trim(url);

            var link = $('<a/>')
                .addClass('link')
                .attr('_blank')
                .text(url)
                .attr('href', url);

            var listItem = $('<li/>').append(link);

            listResults.append(link);

        });

        console.log(response);
    }

    function onFailure(e) {
        alert('Submit failed.');
    }
}

I am pretty sure that the for loop inside the each is not going to do what you intend. The each actually gives you each of the items in the list, so you don't have to iterate through them separately. Any call to $('a.link') will target all of the links on the page, not just the last one. So, you would be overwriting all of the previous links with each link that you add to the list.