1
\$\begingroup\$

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?

\$\endgroup\$
4
\$\begingroup\$

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;
            }
        }
    }
    
\$\endgroup\$
  • \$\begingroup\$ Ty. Is Dapper like dotConnect? \$\endgroup\$ – Spedo De La Rossa Mar 6 '18 at 21:25
  • \$\begingroup\$ Disagree with avoiding ADO.net; sometimes it's better to use something higher level, sometimes it's not. Depends on the use case. \$\endgroup\$ – 404 Mar 28 '18 at 16:37
4
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – t3chb0t Mar 6 '18 at 11:30
  • \$\begingroup\$ 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. \$\endgroup\$ – Nkosi Mar 6 '18 at 11:32
0
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.