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?

• Daryl: Not sure why an editor added "Grabbing SQL records:" to the title? The fact that the data is coming from a SQL table is immaterial.
• Mathieu Guindon: Good titles should say (or hint at) what the code does; that addition made your title more specific.

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.

• Daryl: Great advice (I have no one to run code by at the office, so I figured I'd give this a try; have never done code reviews... sigh).
• Daryl: So would a Try...Catch have any benefit in there?
• Mathieu Guindon: The best exception handling is writing code that doesn't throw exceptions in the first place. Catching exceptions that can be otherwise avoided, is hurting performance. I don't see any benefit for a try/catch here, no.