5
\$\begingroup\$

I need help in improving this script that I have written to parse a URL and check it from an array.

This is what I have done till now: (fiddle)

This is working fine. Was this done correctly? What exactly I need is to search URLs in a list (ul) (which will be a domain specific), and then match if the URL is found in an array (array will have 500+ domains). If it is found then I need to append some text.

Is this function fine to use if there is a long list of domain names in a document and an array has 100+ domains?

Should I use regex to get the domain or is finding it from a CSS selector a good idea?

<div id="links">
    <ul>
        <li>
            <a href="http://www.azaz.com">Link 1</a>
        </li>
        <li>
            <a href="http://www.azaz.info">Link 2</a>
        </li>
        <li>
            <a href="http://www.123.com">Link 3</a>
        </li>
        <li>
            <a href="http://www.566.com">Link 4</a>
        </li>
        <li>
            <a href="http://www.890.com">Link 5</a>
        </li>
        <li>
            <a href="http://www.azaz.com">Link 6</a>
        </li>
        <li>
            <a href="http://www.azaz.info">Link 7</a>
        </li>
        <li>
            <a href="http://www.123.com">Link 8</a>
        </li>
    </ul>
</div>



var arr = ['http://www.azaz.com', 'http://www.123.com'];
        $("#links li").each(function () {
        var aa = $(this).find('a').attr('href');
        var found = $.inArray(aa, arr) > -1;
        if(found === true){
            $(this).append("true");
        }
});
\$\endgroup\$
  • \$\begingroup\$ Welcome, Kannu. Codereview is about improving fully functional code. If you need help with implementation and/or are looking for suggestions on unwritten code those are outside the realm of this site(read: edit those out). I hope you get some fine answers! \$\endgroup\$ – Legato Apr 20 '15 at 19:33
  • \$\begingroup\$ This is actually my fully functional code of my concept and it is working. Need help in improving it. \$\endgroup\$ – kannu Apr 20 '15 at 19:45
  • \$\begingroup\$ Yes, I understand that, I'm referring to your "need help with x" introduction, which could be confusing. \$\endgroup\$ – Legato Apr 20 '15 at 19:49
  • 1
    \$\begingroup\$ true, let me edit it. lol. I am little bit poor in content writing in english. :( \$\endgroup\$ – kannu Apr 20 '15 at 20:06
3
\$\begingroup\$

I agree that your solution is the right way to do what you want, but I would add that it can be a bit compressed for cleaner coding, like this:

var arr = ['http://www.azaz.com', 'http://www.123.com'];
$("#links li").each(function () {
    if( $.inArray($(this).find('a').attr('href'), arr) > -1) {
        $(this).append("true");
    }
});

This way we're using less statements and no variable at all but the code keeps easily readable.

\$\endgroup\$
3
\$\begingroup\$

Your code is simple and works, so I don't see any need to over-complicate things. It ofcourse depends on the sizes of the lists/data. But 500 doesn't seem so bad, performance wise.

It's good that you decided not to use regular expressions ([source])1 Using a CSS selector seems more appropriate. However it's prone to end up in errors if the site that provides the list changes its css.

In addition, I believe you have a small mistake with your indentation. Perhaps it's just a copy/paste error.

var arr = ['http://www.azaz.com', 'http://www.123.com'];
$("#links li").each(function () {
        var aa = $(this).find('a').attr('href');
        var found = $.inArray(aa, arr) > -1;
        if(found === true){
            $(this).append("true");
        }
});

instead of

var arr = ['http://www.azaz.com', 'http://www.123.com'];
        $("#links li").each(function () {
        var aa = $(this).find('a').attr('href');
        var found = $.inArray(aa, arr) > -1;
        if(found === true){
            $(this).append("true");
        }
});
\$\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.