Xaisoft
Xaisoft

Reputation: 46651

Why is this bad code?

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?

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

Upvotes: 4

Views: 634

Answers (6)

James
James

Reputation: 82136

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;
     }
}

Upvotes: 1

leppie
leppie

Reputation: 117350

Besides the given answers, the method is actually named confusingly, now the maintainer has to dig around to figure out what it does.

Upvotes: 3

Michael Gattuso
Michael Gattuso

Reputation: 13220

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.

Upvotes: 5

Charles Bretana
Charles Bretana

Reputation: 146603

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.

Upvotes: 1

bleeeah
bleeeah

Reputation: 3604

Because he returns the same string he passed into the method.

Upvotes: 2

Robban
Robban

Reputation: 6802

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

Upvotes: 17

Related Questions