6

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?

Improve this question
1
  • 3
    I really don't think you want the compiler making up arbitrary ToString( ) implementations for you. Commented Jun 29, 2009 at 0:58

8 Answers 8

Reset to default
10

By creating an override of ToString in the IntrestRate class.

Improve this answer
Sign up to request clarification or add additional context in comments.

3 Comments

Not the way I would have worded it, but this answer is correct... Classes that have some meaningful way of representing themselves as a string should always override ToString().
Fair enough, but I think the question is actually how do you pick up this kind of a mistake? He could have changed the definition for the InterestRate property months after having created the PrintQuote method and not realized the implications. As he said, the compiler won't help here. The solution to this is to have a unit test set up for absolutely every member of every type.
I think getting into the habit of always overriding ToString is a good answer, but I think it tends to be easier to remember to unit test (you can at least run a Test Coverage Analysis, whereas there doesn't seem to be any mainstream tool that will remind you to override ToString).
4

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.

Improve this answer

Comments

3

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.

Improve this answer

Comments

3

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.

Improve this answer

Comments

1

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. :)

Improve this answer

1 Comment

If you choose to use a common base class, and if you can make that class abstract, there's the possibility of saying public abstract override string ToString();. Then derived classes are forced to re-implement ToString.
0

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.

Improve this answer

Comments

0

For a very different perspective, you could defer all ToString'ing to a separate concern of your application. StatePrinter (https://github.com/kbilsted/StatePrinter) is one such API where you can use the defaults or configure depending on types to print.

var car = new Car(new SteeringWheel(new FoamGrip("Plastic"))); car.Brand = "Toyota";

then print it

StatePrinter printer = new StatePrinter(); Console.WriteLine(printer.PrintObject(car));

and you get the following output

new Car() { StereoAmplifiers = null steeringWheel = new SteeringWheel() { Size = 3 Grip = new FoamGrip() { Material = ""Plastic"" } Weight = 525 } Brand = ""Toyota"" }

and with the IValueConverter abstraction you can define how types are printer, and with the FieldHarvester you can define which fields are to be included in the string.

Improve this answer

Comments

-1

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.

Improve this answer

1 Comment

Actually the problem wouldn't have been picked up even if .ToString() was specified explicitly. I don't think it's worth trading off the usability in this case. Also the interest rate is just an example. There are many cases where the primitive type makes more sense but suffers from the same ToString() problem. Never exposing primitive types though may be a decent enough answer.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.