1
\$\begingroup\$

This is just a simple login form with a very vanilla HTML markup, but I just wanted to get some professional feedback. I know about jslint and try to adhere to there principals where possible.

<script>
//event listeners
$('#sign_in_button').on('click', sign_in);
$('#create_new_account').on('click', create);
$('#create_signup_form').on('click', modal);

function modal() {
    $('#error_box2').text('').hide();
}

function login_success() {
    linkLocation = 'home.html';
    $("body").fadeOut(3000, redirect_page);    
}

function redirect_page() {
    window.location = linkLocation;
}

function login_error() {
    var msg = 'Error logging in';  
    error(msg);
}


function get_form_input(id,validator) {
    var check ='',
        type =$(id).attr('type');

    if(type != 'checkbox' ) {
        check = $(id).val();
    } else if (type == 'checkbox') {
        check = $(id).attr('checked');
    }

    if(validator(check)){
        return check;
    } else {
        return false;
    }
    return false;
}



///validation functions
function validate_username(id) {
    var username_regex = /^[A-Za-z](?=[A-Za-z0-9_.]{3,20}$)[a-zA-Z0-9_]*\.?[a-zA-Z0-9_]*$/;
    var msg;
    if (!username_regex.test(id)) {
        if ((id.length) > 21){
            msg = "under 20 characters.";
        } else if((id.length) < 4) {
            msg = " more than 4 characters.";
        }else {
            msg = "You can use 4-20 characters, including underscores and one dot.";
        }
        return error(msg);
    } 
    return username_regex.test(id);
}

function validate_password(pass) {
    var msg;
    var password_regex = /^.{8,20}$/;

    if (password_regex.test(pass)) {
        return true;
    } else if (pass.length <8) {
        msg = "Passwords should be 8 characters or more.";
        return error(msg);
    } else if (pass.length >21) {
        msg= "twenty character max";
        return error(msg);
    }
    else if (!password_regex.test(password)) {
        msg= "Passwords can be  8-20 characters";
        return error(msg);
    }    
}


function validate_email(email) {
      var email_regex = /^([a-zA-Z0-9_\.\-\+])+\@(([a-zA-Z0-9\-])+\.)+([a-zA-Z0-9]{2,4})+$/;
      var msg;
      if (!email_regex.test(email)) {
          msg = "Not a valid email.";
          return error(msg);
      }
     return email;
}


function error(msg) {
    $('#error_box1').slideDown();
    $('#error_box1').text(msg);

    return false;
}

function create_success() {
    login_success();
}

function create_error() {
    var msg = 'Error logging in';  
    error(msg);
}


function sign_in() {
    var send = {
        'password' : get_form_input('#password', validate_password),
        'username' : get_form_input('#username', validate_username),
    };
    var url = "http://";

    if (send.username && send.password) {
    console.log(send); 
        $.when($.post(url,send)).then(login_success, login_error);
    } else {
        return;
    }

}

function create() {
    var send = {
        'password' : get_form_input('#new_password', validate_password),
        'username' : get_form_input('#new_username', validate_username),
        'email'    : get_form_input('#new_email', validate_email)
    };


    var url = "http://";

    if (send.username && send.password && send.email) {
        console.log(send); 
        $.when($.post(url,send)).then(create_success, create_error);
    } else {
        //return error('Error logging in. Try again soon.');
    }
}


</script>
  1. What is the non-quick fix for the name space issue?
  2. Why couldn't a heavy, more complicated application not function effectively with procedural code?
\$\endgroup\$
  • \$\begingroup\$ "As professionals, is there anything clearly amateurishness about this code that I could improve?" - the over-use of $() \$\endgroup\$ – rlemon Aug 15 '12 at 12:53
  • \$\begingroup\$ To clarify, you shouldn't just reach into the DOM every time to select stuff. cache it (store it as a local variable) ... var el = $('#error_box1'); el.slideDown().text(msg); due to chaining this is a bad example \$\endgroup\$ – rlemon Aug 15 '12 at 13:06
  • \$\begingroup\$ Not worthy of "answer" status: generally, JavaScript code is camelCased rather than using underscores. Obviously styles differ, but you asked ;). Also, $(id).attr('checked') should be $(id).prop('checked'), as attr() only looks for the HTML attribute, not whether the checkbox is actually checked. \$\endgroup\$ – Heretic Monkey Aug 15 '12 at 18:49
3
\$\begingroup\$
  • It's in the HTML (script tag). Link it. Keep 'em separated whenever possible.

  • Not a big deal but when declaring vars you can do them with one var statement, e.g. var a = 0, b = 3;

  • Add 'use strict;' to look super cool (but I've seen at least one thing that will break)

  • Fix stuff like linkLocation = 'home.html'; (no var statement - would be global - illegal in strict mode)

  • window is global in browsers. window.location can just be called as location

  • If this is to be seen by interviewers, be consistent with your style. For instance, you mix ' and " for strings. Most prefer ' to differentiate from HTML attribute quotes which are typically double. Mixing both looks like it's not all your code or you're a little sloppy.

  • When it's just a JavaScript object and not JSON, it doesn't need quotes on the property names. e.g. send = { 'password':...

  • If it's an interview situation, don't pretend any of the regEx is yours unless you can explain it.

  • You're pooping all over the global namespace - quick fix would be:

    (function(){

    //wrap your code here

    })()

Don't know what's wrong with code formatting but that puts your functions in scope with anonymous function that automatically fires so it should keep your stuff from being overwritten or overwriting other stuff in the global namespace.

Otherwise it looks like good, clean, straightforward, procedural code. Nothing wrong with that when it's all that's called for. If this were a heavy, complicated, application, I'd be wondering where the objects were at.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for you're response. 2 questions: 1) The name space issue, vis-a-vis pooping all over it, what is the non-quick fix? 2) Why couldn't a heavy, more complicated application not function effectively with procedural code? Thanks \$\endgroup\$ – Emin Israfil Aug 14 '12 at 2:44
2
\$\begingroup\$

Polluting the global scope with variables is not a wise idea. Instead, lets pass in linkLocation as a parameter:

function login_success() {
    $("body").fadeOut(3000, function(){
        redirect_page('home.html');
    }); 
}

function redirect_page(linkLocation) {
    window.location = linkLocation;
}

If looks like you may have a bug with get_form_input. If the form input is a checkbox and is unchecked it will return false even if it passes the validator function. Since this will cause the sign_in function to fail, lets return null as an invalid state.

function get_form_input(id,validator) {
    var check,
        type =$(id).attr('type');

    if(type != 'checkbox' ) {
        check = $(id).val();
    } else if (type == 'checkbox') {
        check = $(id).attr('checked');
    }

    return validator(check) ? check : null;
}

To guard agains the previously mentioned bug, we need to check if not null. I'm including only the create function.

function create() {
    var send = {
        'password' : get_form_input('#new_password', validate_password),
        'username' : get_form_input('#new_username', validate_username),
        'email'    : get_form_input('#new_email', validate_email)
    };

    var url = "http://";

    if (send.username != null && send.password != null && send.email != null) {
        console.log(send); 
        $.when($.post(url,send)).then(create_success, create_error);
    } else {
        //return error('Error logging in. Try again soon.');
    }
}

Regular expressions are tough to read (human wise). As such, if you have a gnarly reg-ex in your code (username_regex), you should comment what it does. Furthermore, using regular expressions to check the total length of the string is inefficient and not necessary here, since you already have to check the length to report an accurate error message. As such, consider validating the username and password like this:

function validate_username(id) {
    /* Starts with an alpha character and contains at most one '.' */
    var username_regex = /^[A-Za-z]\w*\.?\w*$/;
    var msg;
    if (id.length > 21){
        msg = "under 20 characters.";
    } else if (id.length < 4){
        msg = " more than 4 characters.";
    } else if(!username_regex.text(id)){
        msg = "You can use 4-20 characters, including underscores and one dot."
    } else {
        return true;
    }

    return error(msg);
}

function validate_password(pass) {
    var msg;

    if (pass.length < 8) {
        msg = "Passwords should be 8 characters or more.";
    } else if (pass.length > 21) {
        msg = "Passwors may be twenty character max.";
    } else {
        return true;
    }  

    return error(msg);
} 
| improve this answer | |
\$\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.