Why is this bad code?

Question

I was considering how to mess up your worst code ever written, and I'm not quite sure due to my lack of knowledge about why this is bad code.

public string GetUsername (string userName) { User user = DbLookup.GetUser(userName); return user.Username; } 

Is it because it assumes that username will exist and not check for null ? Or is there anything else?

https://stackoverflow.com/questions/130965/what-is-the-worst-code-youve-ever-written/191969#191969

Answer 1

because it returns the same thing that the user sends as input to the method ... Username

Answer 2

It does not return the same string that was provided. It returns the username from the database, and the user may or may not exist - this way, it can return null. The name of the method may be incorrect, given what it does. Someone from the original post noted that this should be the name of the CheckIfUsernameExistsAndReturn sorta method.

Answer 3

In addition to these answers, the method is actually called confused, now the maintainer must dig in to find out what he is doing.

Answer 4

Because it returns the same string as in the method.

Answer 5

because if the user exists, it simply returns the same value that is passed as the method parameter, and if the user does not exist, it will throw a null-reference exception.

Answer 6

As already mentioned, this is bad code simply because the method is really redundant. It returns (provided that User.Username matches the parameter) the same parameter value. However, since you mentioned another reason, this is bad because it does not verify that the User is null before he tries to return the username.

Another potential problem: GetUser may throw an exception that is not handled in the method (it can actually be handled externally or internally). Just a thought ...

An improvement would be to return a User object, not a username:

 public User GetUser(string username) { try { return DBLookup.GetUser(username); } catch (DBLookupException ex) { // throw exception or handle exception return null; } }