3
\$\begingroup\$

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?

\$\endgroup\$
2
  • \$\begingroup\$ 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. \$\endgroup\$
    – Daryl
    Jul 8 '16 at 12:13
  • \$\begingroup\$ Good titles should say (or hint at) what the code does; that addition made your title more specific. \$\endgroup\$ Jul 8 '16 at 13:18
5
\$\begingroup\$
// 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.

\$\endgroup\$
3
  • \$\begingroup\$ 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). \$\endgroup\$
    – Daryl
    Jul 7 '16 at 19:01
  • \$\begingroup\$ So would a Try...Catch have any benefit in there? \$\endgroup\$
    – Daryl
    Jul 7 '16 at 19:02
  • 3
    \$\begingroup\$ 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. \$\endgroup\$ Jul 7 '16 at 19:03

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.