5
\$\begingroup\$

I'm working on a script that will be able to press the login button on any site for an app I'm working on. I have it working (still a few edge cases to work out such as multiple submit buttons and iFrames and I'm going to rewrite it to make it prettier once I figure them out).

Can you think of any ways I could improve it or make it more robust?

Some notes:

  • form.submit() doesn't work consistently since many sites don't allow it to be executed via a script.
  • Three types of login buttons I see consistently are 'a', 'button', and 'input'.

var buttonElements = document.getElementsByTagName('button');
var inputElements = document.getElementsByTagName('input');
var linkElements = document.getElementsByTagName('a');
var loginElement = null;
// For Button/Input
if (buttonElements != null){
    loginElement = getSubmitElement(buttonElements);
}
if (loginElement != null){
    console.log(loginElement);
    loginElement.click();
}
else{
    if (inputElements != null){
        loginElement = getSubmitElement(inputElements);
    }
    if (loginElement != null){
        console.log(loginElement);
        loginElement.click();
    }
    else{
        if (linkElements != null){
            loginElement = getLinkElement(linkElements);
        }
        if (loginElement != null){
            console.log(loginElement);
            loginElement.click();
        }
    }
}
// Used for link elements (<a/>)
function getLinkElement(elements){
    for (i=0;i < elements.length; i++){
        // Is a submit button
        // Check ID class
        if (isLoginElement(elements[i],'id')){
            return elements[i];
        }
    }
    for (i=0;i < elements.length; i++) {
        if(isLoginElement(elements[i], 'name')){
            return elements[i];
        }
        else if(isLoginElement(elements[i],'class')){
            return elements[i];
        }
    }
    return null;
}

// Use for Button or Input - Checks the id/name/class attributes
function getSubmitElement(elements){
    for (i=0;i < elements.length; i++){
        var type = elements[i].getAttribute('type');
        if (type != null){
            if (type.toLowerCase() == 'submit' || type.toLowerCase() == 'image'){
                // Is a submit button
                // Check ID class
                if (isLoginElement(elements[i],'id')){
                    return elements[i];
                }
                else if(isLoginElement(elements[i], 'name')){
                    return elements[i];
                }
                else if(isLoginElement(elements[i],'class')){
                    return elements[i];
                }
            }
        }

    }
    return null;
}


// Checks if a specific attribute value contains our preset login strings
function isLoginElement(element, attribute){
    var attr;
    if (attribute != 'content'){
        attr = element.getAttribute(attribute);
    }
    else{
        attr = element.textContent;
    }
    if (attr != null){
        if (hasLogin(attr)){
            return true;
        }
    }
    return false;
}

// Checks that an attribute's value contains login, sign, or submit
function hasLogin(str){
    var substrings = ['login', 'sign', 'submit', 'continue'];
    if (new RegExp(substrings.join("|")).test(str.toLowerCase())) {
        console.log('Match Found - ' + str);
        return true;
    }
    return false;
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for the tags, I'm new to web development (C# background) \$\endgroup\$ – Levi Fuller Feb 1 '15 at 19:42
2
\$\begingroup\$

In general, it seems to me that you're overengineering this (and obfuscating your code by doing so).

For example:

  • Your hasLogin() function is only used in one place, and could be replaced by a simple one-liner there, e.g. like this:

    return /login|sign|submit|continue/i.test( attr );
    

    (Yes, this works as expected even if attr is null. BTW, note that this isn't an anchored match, so it will match e.g. "noLogin" or "starsign". This is true for your original code as well, but it's harder to see there.)

  • Most of the complexity in your isLoginElement() function is there to implement the 'content' feature, which you don't even use. Per YAGNI, you shouldn't implement such features until you actually need them. With that unused feature removed, your isLoginElement() can be simplified to one or two lines of code:

    function isLoginElement( element, attrName ) {
        var attr = element.getAttribute( attrName );
        return /login|sign|submit|continue/i.test( attr );
    }
    

    If you did end up needing that feature later, it might be best implemented in a wrapper function for element.getAttribute(). Or, better yet, you could get rid of isLoginElement() entirely, and just have a one-line function that takes in a string and tests it against a regexp, just like your (kind of poorly named, IMO) hasLogin() did. Then you could replace the code that currently calls isLoginElement() with something like:

    if ( isLoginString( elements[i].id ) ) return elements[i];
    if ( isLoginString( elements[i].name ) ) return elements[i];
    if ( isLoginString( elements[i].className ) ) return elements[i];
    

    Actually, you don't even really need the isLoginString() function, since you can just save the regexp in a variable and test each attribute directly against it:

    var loginRegexp = /login|sign|submit|continue/i;
    // ...
    if ( loginRegexp.test( elements[i].id ) ) return elements[i];
    if ( loginRegexp.test( elements[i].name ) ) return elements[i];
    if ( loginRegexp.test( elements[i].className ) ) return elements[i];
    
  • You could further simplify you code by using document.querySelectorAll() instead of document.getElementsByTagName(), which would let you reduce your whole code down to e.g.:

    var loginRegexp = /login|sign|submit|continue/i;
    
    function findLoginElement ( selector ) {
        var elements = document.querySelectorAll( selector );
        for ( var i = 0; i < elements.length; i++ ) {
            var element = elements[i];
            if ( loginRegexp.test( element.id ) ) return element;
            if ( loginRegexp.test( element.name ) ) return element;
            if ( loginRegexp.test( element.className ) ) return element;
        }
        return null;
    }
    
    var loginElement = 
        findLoginElement( 'button[type=submit], button[type=image]' ) ||
        findLoginElement( 'input[type=submit], input[type=image]' ) ||
        findLoginElement( 'a' );
    
    if ( loginElement ) {
        console.log( 'found login element', loginElement );
        loginElement.click();
    }
    

    Note: One minor difference between this and your original code is that your code prefers links (but not buttons) with id matches over links with other types of matches. It's easy enough to implement that, if you want, although the discrepancy between the handling of links and other elements makes code reuse a bit harder. (One fairly simple way would be to add a second for loop inside findLoginElement() that only checks id attributes and skips any elements that aren't links, and/or is only executed if the selector is "a".)

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks mate, this helped me out a ton! I didn't know you could implement a regex like that. Just beautiful, my friend. \$\endgroup\$ – Levi Fuller Feb 4 '15 at 3:35
2
\$\begingroup\$

I have a couple of thoughts on this in terms of coding style, but not really regarding robustness. I hope you'll get more in-depth reviews in terms of robustness too!

Simplifying the complex if-else chain of loginElement

In this code:

var loginElement = null;
if (buttonElements != null){
    loginElement = getSubmitElement(buttonElements);
}
if (loginElement != null){
    console.log(loginElement);
    loginElement.click();
}
else{
    // ...
}

Notice that if buttonElements is null, then loginElement will still be null after the first if statement, in which case the second if will be pointless. This is equivalent to the above but without the wasted step:

var loginElement = null;
if (buttonElements != null){
    loginElement = getSubmitElement(buttonElements);
    if (loginElement != null){
        console.log(loginElement);
        loginElement.click();
    }
}
else{
    // ...
}

On closer look, the entire if-else chain can be simplified to this:

var loginElement = null;

if (buttonElements != null) {
    loginElement = getSubmitElement(buttonElements);
} else if (inputElements != null) {
    loginElement = getSubmitElement(inputElements);
} else if (linkElements != null) {
    loginElement = getLinkElement(linkElements);
}

if (loginElement != null) {
    console.log(loginElement);
    loginElement.click();
}

Cache duplicated operations in a local variable

In this statement, in the worst case, the type variable will be lowercased twice:

if (type.toLowerCase() == 'submit' || type.toLowerCase() == 'image'){

It would be better to lowercase only once and store in a local variable:

var normalizedType = type.toLowerCase();
if (normalizedType == 'submit' || normalizedType == 'image') {

Simplify else-if chains using early returns

When you return in the if-else-if branches like this:

if (isLoginElement(elements[i],'id')){
    return elements[i];
}
else if(isLoginElement(elements[i], 'name')){
    return elements[i];
}
else if(isLoginElement(elements[i],'class')){
    return elements[i];
}

You can drop the else, which makes it slightly simpler:

if (isLoginElement(elements[i],'id')) {
    return elements[i];
}
if (isLoginElement(elements[i], 'name')) {
    return elements[i];
}
if (isLoginElement(elements[i],'class')) {
    return elements[i];
}

Other simplification

This code:

if (attr != null){
    if (hasLogin(attr)){
        return true;
    }
}
return false;

Can be written simpler as:

return attr != null && hasLogin(attr);
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I appreciate the answer, mate. I'm looking for suggestions more so from a design perspective (not even code). I wrote the code above real-time (going through a site one step at a time and altering it in order to just get it to work (proof of concept). Do you see anything in my code design aspect which could potentially make it more robust in the sense that I can easily factor in other things (such as a new tagname or something like that). Scalable and unbreakable. I will make sure to implement your suggestions though, janos! \$\endgroup\$ – Levi Fuller Feb 1 '15 at 20:38
  • \$\begingroup\$ @LeviFuller you're quite right. I added a note at the very top to signal to other reviewers that you still need input on the robustness. I hope you'll get some. Or I'll try to take a deeper look later when I have a bit more time. \$\endgroup\$ – janos Feb 1 '15 at 20:48

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.