Monday, February 18, 2013

[Obsolete] should be obsolete

[Obsolete] attribute is a pretty useful concept in the .NET Framework, designed to mark parts of our code that is planned to be phased out and its usage should be avoided. It makes a lot of sense to mark methods or types as obsolete, e.g. when we drop support for some parts of our API and the code is planned to be removed when all the customers are migrated, or when we've restructurized our code so that there's a newer approach for particular problem and we just had no time or possibility to upgrade all the usages, etc.

Recently, in the project I work in I've stumbled upon a pretty useful class, being part of our project-wide infrastructure and heavily used through the project, marked with an [Obsolete] attribute. Unfortunately, the parameterless overload, without a message, was used. I didn't know that part of code well, so I decided to consult with the author what does it mean for this class to be obsolete. Quick research through the revision history traced back to a developer who is no longer working in the project. Also, noone in the team could point me to an alternative approach I should use, most probably because there was none yet.

I've lost some time trying to figure out what's the reason of that spooky [Obsolete] attribute and I've only came to the conclusion that the developer who put it there must have been planning some redesign or was just unhappy with the class as it is. And even if I'm actually wrong and there is a valid reason for not using the class in question, I had no way to know that, just because someone was too lazy to leave a simple message in the attribute. Without the message, we're just having a pointless compiler warning whenever we're using that class.

To avoid such a confusing situations and a loss of time for our successors, teammates, or even for ourselves, I think we should never define the [Obsolete] attribute using its parameterless constructor. In fact, in my opinion, it should be marked as obsolete itself.

We have such an easy alternative. Wouldn't it be nicer if I had encountered the attribute with a message provided? Like [Obsolete("Use class Foo instead.")] or [Obsolete("Support for LoremIpsum dropped as of v. 3.2")]?

2 comments:

  1. If you're obsoleting just one overload of a method, I think there is often (but not always) a case that you don't need to put in any more explanation

    ReplyDelete
    Replies
    1. But for example if you compile some 3rd party code that is using this overload, you have warning and no sign why something is wrong - you need to dig into an unknown API and understand what are the alternatives. And leaving one sentence like "use Foo(string) instead" doesn't seam to be a fuss.

      Delete