In Suneido object.Delete(key) returns false if the member isn't found, otherwise it returns the object.
Several times we've had bugs resulting from doing things like:
object.Delete(key).Add(...)
which works as expected, except when key isn't found and Delete returns false.
It's likely the worst of possible designs. It would have been better if it returned nothing, or true/false, or my favorite - always the object.
I've been aware of this problem for quite a while but I was hesitant to change it because I was afraid I'd break existing code. I finally decided to go through our code and see if it would actually break much.
There were about 800 uses of Delete.
By far the majority ignored the return value.
I didn't find a single place where we made use of the false return value.
I did find a number of uses which assumed that it always returned the object - i.e. potential bugs.
Other than being tedious, the worst part was seeing all the ugly code. I wonder if it's possible to write code that doesn't make you cringe when you come back to it later.
I found quite a few places where we were doing multiple deletes so while I was at it, I changed Delete to handle multiple arguments.
This is a small change, but I think it's just as important to get the details right as it is to work on the big picture.
1 comment:
I cringe when I look at my code the next morning, let alone years later.
We got taught so many things when we were just starting out. These things were true in the right context, but wrong in other contexts. Unfortunately, when you're just starting out, you don't have the experience to realize the difference.
We were taught, for example, to make our code flexible. I'd suggest that that's why we write functions like you Delete -- it deletes an element, but it also tells you if the element existed.
As your review of the code shows, almost no one wanted one of those functions. Much better to have a Delete that deletes, and a Find (which you probably have), that finds.
We all learn these things the hard way.
Post a Comment