C#: Avoiding Bugs caused by not Overriding ToString

Question

I find the following bug occurring far too often in my code and wondered if anyone knows some good strategies to avoid it.

Imagine a class like this:

public class Quote
{
   public decimal InterestRate { get; set; }
}

At some point I create a string that utilises the interest rate, like this:

public string PrintQuote(Quote quote)
{
    return "The interest rate is " + quote.InterestRate;
}

Now imagine at a later date I refactored the InterestRate property from a decimal to its own class:

public class Quote
{
    public InterestRate InterestRate { get; set; }
}

... but say that I forgot to override the ToString method in the InterestRate class. Unless I carefully looked for every usage of the InterestRate property I would probably never notice that at some point it is being converted to a string. The compiler would certainly not pick this up. My only chance of saviour is through an integration test.

The next time I call my PrintQuote method, I would get a string like this:

"The interest rate is Business.Finance.InterestRate".

Ouch. How can this be avoided?

Answer 1

By creating an override of ToString in the IntrestRate class.

Answer 2

Creating an override of ToString is just one of those things you do for most, if not all, classes. Certainly for all "value" classes.

---

Note that ReSharper will generate a lot of the boilerplate code for you. From:

public class Class1
{
    public string Name { get; set; }
    public int Id { get; set; }
}

The result of running Generate Equality Members, Generate Formatting Members and Generate Constructor is:

public class Class1 : IEquatable<Class1>
{
    public Class1(string name, int id)
    {
        Name = name;
        Id = id;
    }

    public bool Equals(Class1 other)
    {
        if (ReferenceEquals(null, other))
        {
            return false;
        }
        if (ReferenceEquals(this, other))
        {
            return true;
        }
        return Equals(other.Name, Name) && other.Id == Id;
    }

    public override string ToString()
    {
        return string.Format("Name: {0}, Id: {1}", Name, Id);
    }

    public override bool Equals(object obj)
    {
        if (ReferenceEquals(null, obj))
        {
            return false;
        }
        if (ReferenceEquals(this, obj))
        {
            return true;
        }
        if (obj.GetType() != typeof (Class1))
        {
            return false;
        }
        return Equals((Class1) obj);
    }

    public override int GetHashCode()
    {
        unchecked
        {
            return ((Name != null ? Name.GetHashCode() : 0)*397) ^ Id;
        }
    }

    public static bool operator ==(Class1 left, Class1 right)
    {
        return Equals(left, right);
    }

    public static bool operator !=(Class1 left, Class1 right)
    {
        return !Equals(left, right);
    }

    public string Name { get; set; }
    public int Id { get; set; }
}

Note there is one bug: it should have offered to create a default constructor. Even ReSharper can't be perfect.

Answer 3

Frankly, the answer to your question is that your initial design was flawed. First, you exposed a property as a primitive type. Some believe this is wrong. After all, your code allows this ...

var double = quote.InterestRate * quote.InterestRate;

The problem with that is, what is the unit of the result? Interest^2? The second problem with your design is that you rely on an implicit ToString() conversion. Problems with relying on implicit conversion are more well known in C++ (for example), but as you point out, can bite you in C# as well. Perhaps if your code originally had ...

return "The interest rate is " + quote.InterestRate.ToString();

... you would have noticed it in the refactor. Bottom line is if you have design issues in your original design, they might be caught in refactor and the might not. Best bet is to not do them in the first place.

Answer 4

Well, as others have said, you just have to do it. But here are a couple of ideas to help yourself make sure you do it:

1) use a base object for all of your value classes that overrides toString and, say, throws an exception. This will help remind you to override it again.

2) create a custom rule for FXCop (free Microsoft static code analysis tool) to check for toString methods on certain types of classes. How to determine which types of classes should override toString is left as an exercise for the student. :)

Answer 5

Not to be a jerk but write a test case each time you create a class. It is a good habit to get into and avoids oversights for you and others participating in your project.

Answer 6

The way to prevent this kind of problem is to have a unit test for absolutely all your class members, which therefore includes your PrintQuote(Quote quote) method:

[TestMethod]
public void PrintQuoteTest()
{
    quote = new Quote();
    quote.InterestRate = 0.05M;
    Assert.AreEqual(
        "The interest rate is 0.05",
        PrintQuote(quote));
}

In this case, unless you defined a implicit conversion between your new InterestRate class and System.Decimal, this unit test would actually no longer compile. But that would definitely be a signal! And if you did define an implicit conversion between your InterestRate class and System.Decimal, but forgot to override the ToString method, then this unit test would compile, but would (correctly) fail at the Assert.AreEqual() line.

The need for having a unit test for absolutely every class member cannot be overstated.

Answer 7

In the case where ToString is called on something statically typed as an InterestRate, as in your example, or in certain related cases where an InterestRate is cast to Object and then immediately used as a parameter to something like string.Format, you could conceivably detect the problem with static analysis. You could search for a custom FxCop rule that approximates what you want, or write one of your own.

Note that it will always be possible to devise a sufficiently dynamic call pattern that it breaks your analysis, probably not even a very complicated one ;), but catching the lowest-hanging fruit should be easy enough.

That said, I agree with some of the other commenter that thorough testing is probably the best approach to this specific problem.