0
\$\begingroup\$

How can I improve this?

jQuery

$(function() {
    $('button').button();
    $('#tabs').tabs();
    $('.ui-state-error').hide();
    $('.btnlogin').click(function() {
        $('#login .ui-state-error').hide();
        if($('#login_email').val().trim() == ''){
            $('#login_email').focus();
        }
        else if($('#login_password').val().trim() == ''){
            $('#login_password').focus();
        }
        else {
            $.ajax({
                url:'login.php',
                type:'post',
                data: { 
                    email: $('#login_email').val().trim(),
                    password: $('#login_password').val().trim()
                },
                success: function(data) {
                    if(data == 'Success'){
                        window.location = 'main.php';
                    }
                    else {
                        $('#login .ui-state-error p strong').html(data);
                        $('#login .ui-state-error').show();
                    }
                }
            });
        }
    });
});

HTML

 <!DOCTYPE HTML>
<html lang="en">
<head>
    <meta charset="utf-8">
    <title>Social Networking</title>
    <link rel="stylesheet" href="css/ui-lightness/jquery-ui-1.8.23.custom.css" type="text/css">
    <link rel="stylesheet" href="css/reset.css" type="text/css">
    <script src="js/jquery-1.8.0.min.js"></script>
    <script src="js/jquery-ui-1.8.23.custom.min.js"></script>
    <script src="js/login.js"></script>
</head>
<body>
<div id="wrapper">
<section>
    <article>
    <div id="tabs" class="tabs">
        <ul>
            <li><a href="#login">Login</a></li>
            <li><a href="#register">Register</a></li>
        </ul>
        <div id="login">
            <div class="ui-state-error ui-corner-all">
                <p><span class="ui-icon ui-icon-alert"></span>
                <strong></strong></p>
            </div>
            <label for="login_email">E-mail:</label>
                <input id="login_email" type="email">
            <label for="login_password">Password:</label>
                <input id="login_password" type="password">
            <label></label>
                <button class='btnlogin'>Log in</button>
        </div>
        <div id="register">
            <div class="ui-state-error ui-corner-all">
                <p><span class="ui-icon ui-icon-alert"></span>
                <strong></strong></p>
            </div>
            <label for="reg_name">Name:</label>
                <input id="reg_name" type="text">
            <label for="reg_email">E-mail:</label>
                <input id="reg_email" type="email">
            <label for="reg_password">Password:</label>
                <input id="reg_password" type="password">
            <label for="reg_cpassword">Confirm:</label>
                <input id="reg_cpassword" type="password">
            <label></label>
                <button class='btnregister'>Register</button>
        </div>
    </div>
    </article>
</section>
</div>
</body>
</html>
Improve this question
\$\endgroup\$

1 Answer 1

Reset to default
8
\$\begingroup\$

One thing I would recommend is to put all of the login code into a distinct function called login(), for example. Then the click binding would look like this:

$('.btnlogin').click(login)

This would do a few things for you. First, it would improve the readability of the code. Since the login code is more than just a few lines, it will be more clear to anyone who may read the code (including you in the future) what its purpose is.

Second, it will improve the code's reusability. Let's say in the future you want to trigger a login by some means other than just a click -- a return keypress, for example. Then you can just call the same login function without having to rewrite the code.

Lastly, it will be easier to find potential errors if the function is not anonymous because you will be able to see the function's signature (login()) in the stack trace.

I know this is just one small suggestion, but keeping this in mind as you write other code in the future could prevent a lot of headaches.

-- edit --

By the way, jQuery.trim() is meant to be used as $.trim(string) rather than $(stringEl).trim(). You might be able to get away with what you have in some browsers because there is a native trim() method for JavaScript Strings. But in some browsers (IE) that won't work.

So I would recommend changing things like this:

$('#login_email').val().trim()

to this:

$.trim( $('#login_email').val() )
Improve this answer
\$\endgroup\$
1
  • \$\begingroup\$ Added one more suggestion. \$\endgroup\$
    – Jake Stoeffler
    Sep 24, 2012 at 3:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.