5
\$\begingroup\$

I understand that account details should be stored using a much more secure method, but this is only a demonstration script I've made to store login credentials and remember a user wishes to remain logged in using localStorage rather than sessionStorage.

Could anyone show me how I might write this more efficiently please? My main concern is code repetition, so I'd love to move toward a much more object oriented structure.

var bic = bic || {};

bic.checkLogin = function() {
    var loginUser = document.getElementById("loginUser").value;
    var loginPass = document.getElementById("loginPass").value;
    var storedUser = localStorage.Username;
    var storedUser2 = storedUser.replace(/"/g,''); // remove " for plain text display in loginForm
    var storedPass = localStorage.Password;
    var checked = document.getElementById("rememberLogin").checked;

    if(checked == true) {
        localStorage.setItem("loggedIn", "yes");
    } else {
        localStorage.setItem("loggedIn", "no");
    }

    // if Username key exists, and user/pass values match storage entries
    if(localStorage.Username) {
        if(storedUser.match(loginUser) && storedPass.match(loginPass)) {
            bic.notify("loginForm", "Logged in as: " + storedUser2 + "<br /><br /><a id='logout' href=''>Log out?</a>");
            document.getElementById("logout").onclick = function(e){e.preventDefault();bic.logout();};
            console.log("Logged in as: " + localStorage.Username);
        } else {
            bic.loginFailed();
            bic.notify("loginStatus", "Login failed. Please try again.");
            console.log("Failed logins: " + localStorage.failedLogins);
        }
    }
}

// localStorage equiv of cookie
bic.isLoggedIn = function() {
    var a = localStorage.loggedIn;
    var storedUser = localStorage.Username;
    var storedUser2 = storedUser.replace(/"/g,''); // removing " as above

    // does a exactly equal "yes"?
    if(a === "yes") {
        bic.notify("loginForm", "Logged in as: " + storedUser2 + "<br /><br /><a id='logout' href=''>Log out?</a>");
        document.getElementById("logout").onclick = function(e){e.preventDefault();bic.logout();};
        console.log("Logged in as: " + localStorage.Username);
    }
}

// logout and remove localStorage key
bic.logout = function() {
    document.getElementById("loginForm").innerHTML = "<input type='text' id='loginUser' placeholder='Username'><br /><br /><input type='password' id='loginPass' placeholder='Password'><br /><br /><button type='button' id='login'>Login</button> <button type='button' id='loginReg'>Register</button><br /><br /><input type='checkbox' id='rememberLogin' /> Stay logged in?<br /><span id='loginStatus'></span>";
    bic.loginPrefill();
    bic.delete("loggedIn");
    console.log("Logged out");
}
\$\endgroup\$
6
\$\begingroup\$

Separate UI from logic

You might want to keep the logic for localStorage handling separate from the DOM fetching logic. That way, your system for handling the local storage can easily be detached and reused elsewhere without that extra baggage. Also works the other way around, when you replace your UI.

Basic login API

In continuation with the previous paragraph, you could model it to have this API:

bic.login(username,password);   // Return true if logged in, false if not
bic.isLoggedIn();               // Return true if someone is logged in, else false
bic.getUserId();                // Gets the current user's ID
bic.logout();                   // Logout current user

bic.register(username,password) // Registration
bic.destroyCurrentUser();       // Just for fun. Only a logged in user can delete himself

The UI logic is separate. It consumes this API rather than being one with it.

LocalStorage is synchronous

Note that localStorage is a synchronous operation. Doing heavy IO against localStorage could mean an unresponsive experience. This has an amplified effect on mobile devices, since they are times slower. Don't pull out or put in huge data. Do it by chunks. Use multiple small keys rather than one big key to house everything your system needs.

JSON.parse() and JSON.stringify()

localStorage is a string-based, key-value storage. But that's it. You need some form of format to structure highly complex data, like user data. Something that may look like:

[
  {id:1,name:"foo",password:"***"},
  {id:1,name:"bar",password:"***"},
  {id:1,name:"baz",password:"***"}
]

To store data like a JS object, you need JSON.stringify() to turn it into a string. Then you can store it to localStorage. To get it, retrieve it from localStorage and then use JSON.parse() to convert from string.

Storage routine

What I typically do when using localStorage is to create an in-memory object of the data. On every save, I serialize the data with JSON.stringify and store it to the localStorage. If ever there are other code that modifies that same data, there are localStorage events to tell you if the data is modified in the storage.

Code refinements

Since I advertised array-based storage, then Array filter, splice, push would be your best friends here. In addition, I believe I introduced you to JSON stringify and parse already.

\$\endgroup\$
  • \$\begingroup\$ This gives me lots to think about moving forward. I'd used JSON.stringify a few times in the remainder of the code. But after your description, I think I've probably used it ineffectively. Many thanks, Josepsh! \$\endgroup\$ – brym Apr 25 '14 at 10:29
2
\$\begingroup\$
if(storedUser.match(loginUser) && storedPass.match(loginPass))

Something like this is a really bad practice. Calling storedUser.match(loginUser) will do a regex evaluation of the value which can provide unexpected results. You should always do something like this:

if(storedUser === loginUser && storedPass === loginPass)
\$\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.