current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 6 down vote favorite

The following extension method is being used in our code base:

public static bool ToBool(this object src)
{
    return src != null && ((string) src).ToBool(false);
}

It leverages off another extension method:

public static bool ToBool(this string src, bool defaultValue)
{
    if (src.IsEmpty())
        return defaultValue;

    if (src.IsNumeric())
        return src.ToInt() != 0;

    var ret = false;
    if (bool.TryParse(src, out ret))
        return ret;
    return defaultValue;
}

(where the ToInt and all the Is* methods are wrappers around the appropriate objects IsNullEmpty / Parse method)

This does not 'feel' right but I cannot explain it to the author. Besides for the obvious problem of passing an object that cannot be cast to a string what other reasons are there to justify this as bad code?

Are there any other concrete reasons why this code may be good or bad?

share|improve this question
1  
This does seem rather strange and unconventional - so you're basically treating null, 0, "", and "0" as false, and all other numbers as true. What is the reasoning behind this? And how is it being used in the rest of the code? –  Saeed Feb 22 '11 at 11:04
    
I think the only reason this code exists is to try and create a more dynamic type system (as mentioned by @martin) or 'fluent' interface. In an attempt to make prettier code. –  NetRunner Feb 22 '11 at 12:32
    
In this case, prettier == confusing. Don't try to turn C# into Ruby, it just won't work well. –  Ed S. Oct 27 '11 at 16:44

2 Answers 2

up vote 5 down vote accepted

I'd criticize:

  • Extension methods on "object" should be avoided because of the broad scope, except for special cases (which such a "ToBool" method is not one). It also feels like an overuse of extension methods.
  • The first one takes an "object", but really wants a string, and will blow up with an InvalidCastException at runtime.
  • The second one does not check for null (which wouldn't be a problem in itself, but having overloads for default values with differing behavior is bad).
share|improve this answer
up vote 5 down vote

Creating and extension method for object that will throw an exception if the actual object isn't a string is asking for trouble.

Also, the general approach looks very much like an attempt to create a dynamic type system where you can use any type and call ToXxx to convert to whatever type you need using "sensible" rules. Is that really what you want in a strongly typed language like C#?

Do you have any guarantees that ToBool will not throw exceptions even if it is passed a string? What if IsNumeric returns true but ToInt throws an exception? None of this code seems to take culture settings into account even though it parses strings to number and may break unexpectedly if run in a different culture.

share|improve this answer
    
All of your points are 100% valid, thanks for the feedback. –  NetRunner Feb 22 '11 at 12:37

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.