Getting the unique identifier from a given login username

Question

Are there any issues in this code?

/// <summary>
/// This function gets the unique identifier of a member by the given
/// login user name of that member on our web site.
/// </summary>
/// <param name="userName">The username</param>
public static string GetMemberNumberByUserId(string userName, string connectionString)
{
 try
 {
 SqlConnection database = new SqlConnection(connectionString);
 SqlCommand command = new SqlCommand("GetMemberInfoByUserID", database);
 command.CommandType = CommandType.StoredProcedure;
 command.Parameters.AddWithValue("@userID", userName);
 database.Open();
 SqlDataReader reader = command.ExecuteReader();
 reader.Read();
 string memberNumber = 
 reader.GetString(reader.GetOrdinal("memberNumber"));
 // Make sure the member number is a 11 or 12 digit number
 if (Regex.IsMatch(memberNumber, @"^(000|100)\d{6,7}(?!00)\d{2}$"))
 {
 return memberNumber;
 }
 else
 {
 return "";
 } 
 }
 catch(Exception)
 {
 return null;
 }
}

• dreza: As mentioned below, not closing the connection. Also I would consider not handling the exception and letting it bubble up instead

Answer 1

catch(Exception)
{
 return null;
}

You should never do this. Exception means that there is some sort of problem. And you shouldn't ignore problems, you should learn about them and do something about them as soon as possible.

What you should do is that at first, don't catch any exceptions. Then, if you find out that some exceptions actually do happen under normal conditions, then handle those, but only those.

Answer 2

As well as what squillman suggested I might look at passing in SQLConnection rather than the connection string. This way the connection could be re-used as it is fairly expensive to create SQLConnections all the time.

Something like:

// Or even better I might consider making this a resource string or injecting it int
// the method as a parameter
static string string MemberNumberFormat = @"^(000|100)\d{6,7}(?!00)\d{2}$";
public static string GetMemberNumberByUserId(SqlConnection connection, string userID)
{
 var command = new SqlCommand("GetMemberInfoByUserID", connection)
 {
 CommandType = CommandType.StoredProcedure
 };
 command.Parameters.AddWithValue("@userID", userID);
 using (SqlDataReader reader = command.ExecuteReader())
 {
 reader.Read();
 string memberNumber = reader.GetString(reader.GetOrdinal("memberNumber"));
 return Regex.IsMatch(memberNumber, MemberNumberFormat)
 ? memberNumber
 : string.Empty;
 } 
}

Then a wrapper class that would be responsible for creating the connection or managinging it's existence. That method for example might look like:

public string GetMemberNumberByUserId(string userID)
{
 var connectionString = string.Empty;
 using (var database = new SqlConnection(connectionString))
 {
 database.Open();
 return GetMemberNumberByUserId(database, userID);
 } 
}

No exception handling is required. The only reason I could see from the code for an exception occurring would be for a major failure. At which point I think you might want to capture that at another level anyway.

• user2730865: could u explain what the difference between passing a sql connection and using connection string is?
• dreza: You have the chance to re-use the sqlconnection in other parts of the code without needing to recreate it as it was created elsewhere of this method
• radarbob: Thumbs up to reusing the Connection object. You'd think SQLxxxx objects were immutable, the way coders never reuse these things. Re-instantiating and building up SQLAdapters over and over again makes me cringe.
• Ingo Bürk: I would go further than this. Don't even pass the connection to methods. Have centralized database object classes that hold the connection as a field rather than having every method accept a connection. On the caller side make sure to only create one such object. Maybe even have that class be responsible for building up the connection to keep the database layer in there.

Answer 3

I think there are several issues with this code. In no particular order...

1. Why is this a static method? While the sql connection string is generally either going to be injected or a dependency, the user name seems likely to be a property of the object. Not a big issue, and in context it could be the right thing.

2. Disposable objects should generally be wrapped in a using block -- unless there is a need for the object to be used after the function is done, which doesn't seem to be the case here.

3. Why are you doing validation in a method called Get...? That should either be done in the database, or after this method.

4. You return an empty string in the case of validation error and null on an exception. You do nothing with your exception handling except return an error -- no logging, no report to the user.

5. You catch an exception at the first point you know what to do to recover from the error -- since this function doesn't do anything with the string it retrieves, how does return null; recover from the error?

Answer 4

One thing is that you never close the SQL connection. I would change that structure a bit and put things inside a using block:

using (SqlConnection database = new SqlConnection(connectionString))
{
 try
 {
 //other code
 }
 catch(Exception)
 {
 //exception code
 }
}

• radarbob: one common 'pattern' is to put Close() in a finally{} block.

Answer 5

I need to know if there are any issues in this code

GetMemberNumberByUserId should not be checking the validity of the returned data. This method should be concerned with only "getting a member number by the user id". This is an example of violating the Single Responsibility Principle.

Exceptions - "covering" database calls with try/catch is idiomatic. To put a fine point on, and perhaps contradict, a couple of previous answers:

1. limit the body of try to the code that you really need to cover. This also means you get specific about the exceptions caught there.
2. catch exceptions as close as practical to the likely source; and database calls is a likely source. Then you can capture lots of good information about your connection, command, etc. objects, parameters passed, etc. and put that stuff in the Exception.Data property.
3. 1 & 2 go hand-in-hand
4. still have an exception catch higher up as suggested by svik.

To illustrate #1 above: do you really want to throw an exception if there is no regex match? Then do not put that in the try block. And it should not be in that method in the first place.