Grabbing SQL records: Nullable bool to bool

Question

So I have a method that does a lookup in an existing List<T>, populated elsewhere by grabbing the records from a SQL table holding configuration data for each state where we do business. This method just looks up in the list by state abbreviation and then returns the value of a Boolean field in the record.

public bool StateRoundsToQuarterHour(int recordID, string stateAbbrev)
    {
        bool? result = null;

        try
        {
            // Query our existing list of StateConfig objects for a match, then return the value of RoundMinutes.
            result = (from x in listStateConfig
                           where x.State == stateAbbrev
                           select x.RoundMinutes).First();  

        }
        catch (Exception e)
        {
            // if state not found, email error and continue
            string strMessage = String.Format("Error: {0} Inner Exception {1}", Convert.ToString(e.Message) + Environment.NewLine, Convert.ToString(e.InnerException));

            // log and email error
            ErrorContinue(recordID, strMessage);

        }

        // as long as we found a result, we're happy
        if(result != null)
        {
            return (bool)result;
        }
        else
        {
            string strMessage = String.Format("State {0} does not exist in table, but RecordID {1} belongs to that state.", stateAbbrev, recordID);

            // log and email error
            ErrorContinue(recordID, strMessage);

            return false;   // if the state doesn't exist, we'll default to rounding is not required
                            // We send an alert about the state not existing, so we'll be able to act on it
        }

    }

The List may not contain a result, so in that case the query will not find anything and remain null. I'm not entirely happy with the method using a nullable Boolean internally and then casting it to non-nullable to use in the return, but I don't want the return type of the method to be nullable. Is this a reasonable approach? Where can this be improved?

Answer 1

// as long as we found a result, we're happy
if(result != null)
{
    return (bool)result;
}

This could be written as simply:

if (result.HasValue)
{
    return result.Value;
}

bool? is shorthand for Nullable<bool>, a generic value type with reference type semantics. Basically Nullable<T>.Value is a T, so there's no need to cast anything.

Also, being a value type, initializing it is redundant.

bool? result;

Is entirely sufficient.

---

Avoid cluttering your code with comments that don't bring anything to the table:

// as long as we found a result, we're happy

Good comments should say why the code does what it does - don't bother commenting just to state the obvious, or to rephrase what the code is already saying.

If this needs a comment every time it's used:

// log and email error
ErrorContinue(recordID, strMessage);

...then perhaps the ErrorContinue method needs a better, more descriptive name?

---

You're catching a System.Exception here:

    try
    {
        // Query our existing list of StateConfig objects for a match, then return the value of RoundMinutes.
        result = (from x in listStateConfig
                       where x.State == stateAbbrev
                       select x.RoundMinutes).First();  

    }
    catch (Exception e)
    {
        //...
    }

But it looks like you're only handling the InvalidOperationException that .First() would throw when there aren't any items matching the where criteria.

If the query is meant to return only 1 record, use .Single(), not .First(). And if the query can return no records, use .SingleOrDefault().

The problem is that you're projecting the result and Selecting .RoundMinutes, which is a value type and can't be null, so SingleOrDefault() would return false for an bool value, because default(int) == 0.

You could instead select the actual StateConfig object, and boil the code down to this:

public bool StateRoundsToQuarterHour(int recordId, string stateAbbrev)
{
    var config = _stateConfig.SingleOrDefault(x => x.State == stateAbbrev);
    if (config != null)
    {
        return config.RoundMinutes;
    }
    else
    {
        var message = string.Format("State '{0}' does not exist in table, but RecordId {1} belongs to that state.", stateAbbrev, recordId);
        _logger.Warn(message);
        return false;
    }
}

Notice Id vs. ID, _logger.Warn vs ErrorContinue, message vs. strMessage, and _stateConfig vs listStateConfig. Don't try to encode the type of a variable into its identifier name, that's bad Hungarian notation, and it's not the type that's useful but what the variable it actually used for.

I'm prefixing private fields (I assumed listStateConfig was a private field here) with an underscore, but you could alternatively qualify them with the this keyword - that's personal preference.