I was looking at a question on here about confessing your worst code ever written and I am not quite sure, because of my lack of knowledge on why this is bad code.
public string GetUsername (string userName)
{
User user = DbLookup.GetUser(userName);
return user.Username;
}
Is it because, it assumes username will exist and doesn't check for null? Or is there more to it?
http://stackoverflow.com/questions/130965/what-is-the-worst-code-youve-ever-written/191969#191969
because it returns the same thing that the user sends as input to the method... Username
because if user exists, it just returns the same value submitted as method parameter, and if user does not exist, it will throw a null reference exception.
It doesn't return the same string that was provided. It return the username from the database and the user may or may not exist - thus it could return null. The method name is perhaps incorrect given what it does. Someone in the original post mentioned that it should be CheckIfUsernameExistsAndReturn sorta method name.
Besides the given answers, the method is actually named confusingly, now the maintainer has to dig around to figure out what it does.
As already stated, it is bad code simply due to the fact the method is redundant really. It is returning (assuming User.Username is the same as the parameter) the same value of the parameter. However, as you mentioned another reason it is bad is because it doesn't check that User is null before it attempts to return the Username.
Another potential issue is GetUser may raise an exception that is not being handled in the method (it could indeed be getting handled externally or internally). Just a thought tho...
An improvement would be to return the User object rather than the username itself:
public User GetUser(string username)
{
try
{
return DBLookup.GetUser(username);
}
catch (DBLookupException ex)
{
// throw exception or handle exception
return null;
}
}