Correct using of try/catch clause on database execution

Question

When a statement to a DB returns null, I am catching it this way:

private int GetLastRoundNumber(int period)
{
 int id;
 try
 {
 using (NpgsqlConnection con = new NpgsqlConnection(_entwConnection))
 {
 con.Open();
 using (NpgsqlCommand cmd = new NpgsqlCommand())
 {
 cmd.Connection = con;
 cmd.CommandText = "SELECT roundNumberId FROM race.folders WHERE period = @period ORDER BY roundNumberId DESC LIMIT 1;";
 cmd.Parameters.AddWithValue("period", period);
 return id = (int)cmd.ExecuteScalar();
 }
 }
 }
 catch (NullReferenceException e)
 {
 Debug.Write(e.Message);
 Debug.WriteLine("Because of a new period, started with 1 again. Period: " + period + ".");
 return id = 1;
 }
}

So to this codepart my questions:

1. Is it okay to have the try/catch block outside the using statement block?
2. Is there a better way to handle the exception?
3. Do I need to hit the catch block?

I am asking because as I learned it by now, for me it isn't a real exception, because when a new period starts, there is no Id inside.

Is this the correct way to deal with it?

Answer 1

Some quick remarks:

• Avoid writing ADO.NET code by hand; instead use an ORM like Dapper.

• Why not use MAX() -- which I think is supported by PostgreSQL -- instead of ORDER BY roundNumberId DESC LIMIT 1?

  SELECT MAX(column_name) 
  FROM table_name 
  WHERE condition; 
  

• Don't put business logic in exceptions. Instead, make this method int? TryGetRoundNumberId() and let the calling code solve the issue when a null is returned.

  Or perhaps provide a default, e.g. int TryGetRoundNumberId(int defaultValue), so you'll get something along these lines:

  using (var sqlConnection = new SqlConnection(_connectionString))
  {
   sqlConnection.Open();
   using (var sqlCommand = new SqlCommand())
   {
   // snip parameters etc.
   var result = (int?)sqlCommand.ExecuteScalar();
   if (result.HasValue)
   {
   return result.Value;
   }
   else
   {
   return defaultValue;
   }
   }
  }
  

• Spedo De La Rossa: Ty. Is Dapper like dotConnect?
• 404: Disagree with avoiding ADO.net; sometimes it's better to use something higher level, sometimes it's not. Depends on the use case.

Answer 2

Just one short note...

catch (NullReferenceException e)

You never want to catch this exception explicitly. 99.999% of the time its occurance means there is a serious bug in your code that can be prevented with proper null checks.

Answer 3

Is it okay to have the try/catch block outside the using statement block?

I see no apparent reason why having a try/catch outside of a using block would cause a problem as disposables wrapped by using blocks will be disposed once they go out of scope regardless of exception or not.

Is there a better way to handle the exception?
Do I need to hit the catch block?

You could avoid that exception entirely by checking the value before trying to return it. And taking action based on what was returned.

private int GetLastRoundNumber(int period) { 
 using (var connection = new NpgsqlConnection(_entwConnection)) {
 connection.Open();
 using (var command = connection.CreateCommand()) {
 command.CommandType = CommandType.Text;
 command.CommandText = "SELECT roundNumberId FROM race.folders WHERE period = @period ORDER BY roundNumberId DESC LIMIT 1;";
 command.Parameters.AddWithValue("period", period);
 var result = command.ExecuteScalar();
 if (result == null) {
 Debug.WriteLine("Because of a new period, started with 1 again. Period: " + period + ".");
 return 1;
 }
 return (int)result;
 }
 }
}

Any exceptions in the above code should bubble up to be handled by the caller. This allows the method to have a single responsibility by not mixing cross-cutting concerns.

• t3chb0t: I'm not a fan of error flags and I find the exception should bubble up to the caller or the method should change its name to TryGetLastRoundNumber to clearly communicate the non-exceptional work.
• Nkosi: Yeah, I was torn on that part and had decided to leave it in. My thought was to avoid handling it in the method on a whole. I'll include your comment in an update.

Answer 4

I don't like using an exception to catch no rows

I would return to a reader

int id;
using (NpgsqlConnection con = new NpgsqlConnection(_entwConnection))
{
 con.Open();
 using (NpgsqlCommand cmd = con.CreateCommand())
 {
 cmd.CommandText = "SELECT roundNumberId FROM race.folders WHERE period = @period ORDER BY roundNumberId DESC LIMIT 1;";
 cmd.Parameters.AddWithValue("period", period);
 using(Reader rdr = cmd.ExectuteReader()) 
 {
 if(rdr.HasRows())
 { 
 rdr.Read();
 id = rdr.GetInt(0);
 }
 else 
 { 
 id = 1;
 }
 }
 }
} 
return id;

just return id once at the end.

The try could be inside the using. using (NpgsqlConnection con = new NpgsqlConnection(_entwConnection)) will never throw an error.

But now you you don't need to catch any errors so you don't need try catch.