Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have the below C# code for parsing. Do you think this is most optimize or I should use generic method or optimization required in these functions themselves?

public static bool GetDBBool(object value)
{
    var result = false;

    if (value != null && string.IsNullOrEmpty(value.ToString()) == false)
        bool.TryParse(value.ToString(), out result);

    return result;
}

public static int GetDBInt(object value)
{
    var result = -999;

    if (value != null && string.IsNullOrEmpty(value.ToString()) == false)
        int.TryParse(value.ToString(), out result);

    return result;
}

public static double GetDBDouble(object value)
{
    var result = -999.00;

    if (value != null && string.IsNullOrEmpty(value.ToString()) == false)
        double.TryParse(value.ToString(), out result);

    return result;
}

public static DateTime GetDBDate(object value)
{
    var result = DateTime.Now;

    if (value != null && string.IsNullOrEmpty(value.ToString()) == false)
        DateTime.TryParse(value.ToString(), out result);

    return result;
}

public static DateTime? GetDBNullableDate(object value)
{
    DateTime date;
    if (value != null && string.IsNullOrEmpty(value.ToString()) == false)
        return DateTime.TryParse(value.ToString(), out date) ? date : (DateTime?)null;
    else
        return null;
}
share|improve this question
3  
These methods seem like a pointless abstraction. They don't offer anything over the .NET TryParse methods. –  Phil K Jul 12 at 14:02
add comment

3 Answers 3

== false is not very readable. Use ! instead.

If the "object value" is already of desired type (e.g. DateTime), it would be slower to convert it to string and then back again.

I would write instead

if (value is DateTime) return (DateTime)value;

I would not use var result = -999; as a return value.

If the conversion fails, you can either return Int32.MinValue (better than -999) or rather null.

Maybe also you would like to check for DBNull.Value? So first I would check for null and DBNull.Value, then "value is DateTime" check and the last thing would be DateTime.TryParse.

share|improve this answer
add comment

Such default values don't really make sense if the value is not valid. Imagine if you call the function like this

 GetDBDouble("-999.00")

You will get -999.00 , but how would you know if its -999.00 indeed or it wasn't a valid conversion. Instead you make your functions return Nullable value type instead

public static int? GetDBInt(object value)
 {
    int result;

    if (value != null && !string.IsNullOrEmpty(value.ToString()))
          if (int.TryParse(value.ToString(), out result))
           {
                return result;
           }
     return null;

  }

And now the caller can call the function like this

var result = GetDBInt("33");
Console.WriteLine(result.HasValue ? result.ToString() : "Invalid value");

The HasValue property will tell whether there is a value or not

share|improve this answer
add comment

More often than not, returning a value in the case of an error is flat out wrong. This applies to all the methods, but as an example let's consider GetDBInt alone.

public static int GetDBInt(object value)
{
    var result = -999;

    if (value != null && string.IsNullOrEmpty(value.ToString()) == false)
        int.TryParse(value.ToString(), out result);

    return result;
}

First, the method hides what it's really doing in the signature. Imagine the following situations:

int value = 5;
int result = GetDBInt(5);

From an optimization point of view, this is silly. You're taking a value that's already of the correct type and converting it to a string so you can convert it back again. Pointless.

How about this:

double value = 5.1;
int result = GetDBInt(value);

In this case, TryParse will fail because 5.1 is not a valid int. What you probably don't realize is that your method WILL NOT return -999. The TryParse method WILL assign a value to the output parameter regardless of the the fact that it failed to parse the string. The actual return value in this case will be 0 (the default value of int).

Okay, so now let's consider what you're probably using these methods for, to parse strings.

string value = "5";
int result = GetDBInt(value);

This case will work as expected, the return result will be 5.

With these things considered, you might find that the methods are not really dependent on "object" types, but in fact expect strings. It might be more appropriate to change the method signatures to take strings instead, that way you are not hiding the fact that the objects are turned into strings first anyway.

At this point, the ToString's can go away, and the null check's become redundant. The method actually reduces to a simple wrapper around TryParse and will return the same results as your original methods. (it will return 0 for null, or any other invalid value).

    public static int GetDBInt(string value)
    {
        int result;
        int.TryParse(value, out result);
        return result;
    }

However, consider the case when the string isn't valid, like so.

string value = "x";
int result = GetDBInt(value);

In this case, we've already determined that the method will return 0 because it's an error. But you're expecting -999 in the case of an error. So let's fix the bug and make sure it returns -999.

    public static int GetDBInt(string value)
    {
        int result;

        if(int.TryParse(value, out result))
            return result;

        return -999;
    }

Okay, so now the method is working the way you intended, but I'd argue that it's still not quite right.

Let's consider our options..

  1. Always treat -999 as an error, but what if -999 is actually in the database as a real value?
  2. Use int.MinValue or int.MaxValue, although less likely, it's the same problem as above.

The other problem with both of those options is that it is not clear to the caller. How are they supposed to know which values are real, and which are not?

  1. You could make the return value a nullable type, and return null on error. Problem is, NULL is sometimes also a valid value in a database.

  2. I propose that the only real solution is to throw an exception. Maybe a FormatException perhaps? Although, in that case you might as well just use int.Parse directly.

    public static int GetDBInt(string value) { int result;

    if(int.TryParse(value, out result))
        return result;
    
    throw new FormatException(string.Format("{0} is not an int", value));
    

    }

  3. There is one option that kind of gives you the best of both worlds. Let the caller decide.

    public static int GetDBInt(string value, int defaultValue) { int result;

    if(int.TryParse(value, out result))
        return result;
    
    return defaultValue;
    

    }

At least this way, the caller can decide what value they want if the database doesn't have it.

string value = "x";
int result = GetDBInt(value, 15);

Lastly, your question is actually about optimization. However, I'm willing to bet that these methods are not the biggest bottleneck in your program. If you ran a profiler over it, they probably won't even show up on the radar. Try not to fall into the premature optimization trap. Focus on working code, and profile it afterwards.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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