Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Below is a script I've written that uses AJAX to check if all links on the page are alive or dead. Could this be modified to be more efficient in any way? Thank you very much.

$(document).ready(function() {
    var liveLinks = [];
    var deadLinks = [];

    function updateLive() {
        $live = Number($('#liveLinkCount').text()) + 1;
        $('#liveLinkCount').text($live);
    }

    function updateDead() {
        $dead = Number($('#deadLinkCount').text()) + 1;
        $('#deadLinkCount').text($dead);
    }

    $('body').prepend("<div id='linkChecker'><span id='hideChecker'>Hide</span><button id='checkLinks'>Check Links</button><br /><span>Live: </span><span id='liveLinkCount' style='color:green;'>0</span><span> - Dead: </span><span id='deadLinkCount' style='color:red;'>0</span></div>");
    $('#linkChecker').css({
        "width": "130px",
        "border": "1px solid black",
        "text-align": "center"
    });
    $('#hideChecker').css({
        "text-decoration": "underline",
        "float": "right",
        "padding": "1px",
        "cursor": "pointer"
    });
    $('body').on('click', '#checkLinks', function() {
        $('#linkChecker').append("<br /><button id='logReport'>Log Report</button>");
        $('a').each(function(i) {
            var self;
            self = this;
            $.ajax({
                type: "post",
                url: $(self).prop('href'),
                success: function() {
                    liveLinks.push($(self).prop('href'));
                    updateLive();
                    $(self).css({
                        "color": "green"
                    });
                },
                error: function() {
                    deadLinks.push($(self).prop('href'));
                    updateDead();
                    $(self).css({
                        "font-weight": "bold",
                        "color": "red"
                    });
                }
            });
        });
    });
    $('body').on('click', '#logReport', function() {
        console.log(new Array(24 + 1).join('\n'));
        console.log("Live Links:");
        for (var i in liveLinks) {
            console.log("  " + liveLinks[i]);
        }
        console.log("Dead Links:");
        for (var i in deadLinks) {
            console.log("  " + deadLinks[i]);
        }
    });
    $('body').on('click', '#hideChecker', function() {
        $('#linkChecker').fadeOut();
    });
});
share|improve this question
    
For readability sake, you can remove the quotes around the JSON properties, i.e.: width: "130px", border: "1px solid black" –  Micah Henning Nov 9 '12 at 20:20
    
Also, I don't really consider using inline styling as good practice. It makes code harder to maintain over time and can make troubleshooting CSS selector specificity more annoying. –  Micah Henning Nov 9 '12 at 20:23
1  
Finally, is there any reason that variables $live and $dead have to be global? –  Micah Henning Nov 9 '12 at 20:25
add comment

2 Answers

up vote 2 down vote accepted

First of all, jQuery's error callback gets invoked on network errors, parser errors, or 4xx/5xx HTTP status codes.

The status codes do not necessarily mean that a link is dead. At least there's a server at the other end. Of course, 404 Not found or 410 Gone are pretty unambiguous, but something like 403 Forbidden could mean that you just aren't allowed to see a certain page - but others might be. Similarly, a 405 Method Not Allowed would mean that your GET request was rejected, because the server expects a POST or something else (normally links mean GET of course, but the page might have some JS that turns a regular link into a POST form submission).

Parser errors could also trip up your system. For instance a link to some malformed XML might cause an error because jQuery might try to parse it, but there is technically some content there, so the link's not exactly dead.

In other words, don't rely on error to always mean "dead link".

Second: Keep a list of URLs you've already checked, and avoid checking the same page twice. You can also do try making some informed guesses. E.g. if a link to "example.com/foo" gets a network error, there a good chance all links to example.com are down. It's not guaranteed of course, but worth considering.

Third: You should ignore links that start with # since they are local to the page you're already on (or their behavior in defined with javascript, in which case you still don't want to check them). You'll also want to avoid those that don't have a href attribute at all, or better yet, report it as a different kind of issue (it's rare and not valid to have such links, but it happens).

Depending on your needs, you could also have an option to skip all "local" links, and only check those that to other sites. E.g. I'm pretty confident that the links to other CodeReview questions on this page are all good, so I might want to limit the checking to external sites only. Or vice-versa.

Fourth: console.log(new Array(24 + 1).join('\n'));
What? I'm pretty sure 24 + 1 will always be 25, so that's one definite optimization you can do. But what do you want with 25 blank lines anyway? Just curious.

Fifth: This is debatable, but you may want to set cache: false in the ajax options. It will slow things down, but you'll also be sure you're checking "for real" and not just getting cached pages.

Sixth: I'd advise you to keep the UI code and the actual checking "engine" more separate. Keeps the code cleaner.

share|improve this answer
add comment

Firstly, for...in is used to enumerate over object properties, not to iterate through arrays. You should use a traditional for loop for this.

Secondly, you could probably refine your console logging portion a bit. It may not have any impact to application performance, but it would make your console look prettier.

$('body').on('click', '#logReport', function() {
    var i, j, k, l;

    console.group("Live Links:");
    for (i=0, j=liveLinks.length; i<j; i++) {
        console.log(liveLinks[i]);
    }
    console.groupEnd();
    console.group("Dead Links:");
    for (k=0, l=deadLinks.length; k<l; k++) {
        console.log(deadLinks[k]);
    }
    console.groupEnd();
});

As far as I know, this works for both Google Chrome and Mozilla Firefox (FireBug). See here for more information.

share|improve this answer
add comment

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.