A simple Java login system

Question

I created a simple log-in system in Java for a project. Security isn't a concern here, hence the plaintext password, but I'm interested in how I can improve the quality of my code. Everything seems to work but it just feels like messy code.

public boolean logIn(String name, String password)
{
 if (getLoggedInUser() != null)
 {
 throw new IllegalArgumentException("You are already logged in");
 }
 boolean login = false;
 for (User user: getUsers())
 {
 if (user.toString().equals(name) && user.getPassword().equals(password))
 {
 loggedInUser = user;
 login = true;
 }
 }
 if (!login) //what if the username + password combo is not correct
 {
 for (User user: getUsers())
 {
 if (user.toString().equals(name))
 {
 throw new IllegalArgumentException("Your password is incorrect");
 }
 }
 throw new IllegalArgumentException("Your username is incorrect");
 }
 return login;
}

• Doi9t: Hello, this question is off-topic, since the code is not compiling; I suggest that you read the [What topics can I ask about here?(codereview.stackexchange.com/help/on-topic) Code Review aims to help improve working code. If you are trying to figure out why your program crashes or produces a wrong result, ask on Stack Overflow instead. Code Review is also not the place to ask for implementing new features.
• forsvarir: @Doi9t I didn't see anything obvious that would stop this from compiling, could you expand on what problem I missed?
• Doi9t: @forsvarir Missing getLoggedInUser and getUsers methods, User object, ect.
• forsvarir: @Doi9t whilst complete, compilable code is easier to review, and can result in more in depth answers, I don't believe it's explicitly required. Possibly out of date meta: codereview.meta.stackexchange.com/a/6372/4203

Answer 1

The Algorithm

The method logIn can be chunked into small steps:

public boolean logIn(String name, String password) {
 if( /* user is logged in already? */)
 for (/* every user */)
 if ( /* has name and password */)
 if (/* no match found */)
 for (/* every user */)
 if ( /* can find user with the given name? */ )
}

The logic is more complex than it has to be. The algorithm searches twice if a user with the name exists. A simpler algorithm could look like:

public boolean logIn(String name, String password) {
 User user = // find user by its name
 if (/* no user found */)
 if (user.isLoggedIn)
 return user.hasPassword(password) 
}

---

Data Structure

The method getUsers() looks like it returns a List. Since it is a list and you do not know at which index a concrete user is saved you have to search for the user.

for (User user: getUsers())
{
 if (user.toString().equals(name))

To check that a user with name does not exists you have to loop through the hole list and this could take some time if you have many users! This is also known as \$O(n)\$ which is a indicator for the time complexity a algorithm can have.

It would be much more performant if we could get a user directly without to search it which would be a time complexity of \$O(1)\$.

We can archive this goal by using a Map instead of a List:

Map<String, User> nameByUser = new HashMap<>();
nameByUser.put("TomZ", new User("TomZ", "aPassw0rt"));
// ..insert some more users
User user = nameByUser.get("TomZ");

Answer 2

Putting security to the side...

A few points

• When you find what you're looking for in a loop, you can exit / return. In your case, rather than login = true, you could just return true;.

• You function returns login, however login is only ever true when it is returned. The function should be void, or return something meaningful, such as the logged in user.

• You're searching through the list twice, once to find the matching user/password and then again to find a matching user so that you can tell them their password is wrong. Really, you only need to search the collection for the user. If it's not there, the name is wrong. If it is, then check the password and either they got it wrong, or they're logged in.

• I'm not sure IllegalArgument is the right exception for this situation. I think of it as something to use if you're passing -1 to a method that expects a positive number. An argument should either be wrong, or right whereas in your code the same arguments could result in an exception or not, depending on what the user has set their password to.

• TomZ: Thanks for the feedback. Which exception type would be better for this situation?
• forsvarir: Something tailored to the situation, so for example docs.oracle.com/javase/7/docs/api/javax/security/auth/login/… seems like one to consider.

Answer 3

toString() instead of getName()

i think you can improve with your validation on names/passwords

user.toString().equals(name) and user.getPassword().equals(password)

i would put this not into the isLoginValid context but more back to your User class (single responsibility - since the User knows its name and can provide information if a given name matches to it's own name)

provide a method for that purpose:

class User {
 ...
 public boolean isNameMatching(String name){...} 
 public boolean isPasswordMatching(String password){...}
}

alternatively it might be useful to provide a validation method using both parameters

if that is too much effort you should at least avoid using the toString() to get the users name.

just an addOn

forsvarir already pointed out some good advices, so this is just a small addOn...