Usage of the Obsolete attribute

Question

I was recently told it was bad practice to haved marked a number of methods in our code with the [Obsolete] attribute. These methods were internal to our codebase, rather than being on an API. The methods handled an older encryption function.

I felt it was a quick and safe way to denote to the rest of the team that these methods should not be used, and provided a message to suggest alternatives.

Others felt that I should have removed the methods entirely, rewriting or refactoring existing code as required. Additionally, it was thought too easy to overlook the compiler warnings.

Is there a 'best practice' for marking code as Obsolete when it's not being used by 3rd parties? Or is this largely subjective?

Answer 1

I would think it is subjective. If it is internal and is a fairly quick process, then I would perform the change.

However, I've also had the situation where the corresponding refactoring took a lot longer (many calls throughout the code base), in which case I used the [Obsolete] attribute. In this case, new development would use the new functions and whoever had time performed refactorings until all calls were gone, which meant that the method could be removed.

Answer 2

It is not a straight forward case. If you remove methods from a working application and refactor the code you create the posibility that you will introduce new bugs and break a working application. If that application is mision critical the impact could be huge and cost the company a lot of money, changes like that would need to be carefully planned and tested. In that case marking methods as obsolete could be worth while, it should help prevent people from using them in further development thus making the eventual refactoring easier. If however the application is not mision critical or the potential for bugs being introduced is low then it may be better to refactor, if you have the time. Ultimately adding the [Obsolete] attribute is a bit like a todo and its use depends on many factors.

Answer 3

Step 1. Mark the member or class as [Obsolete]

Step 2. Update all internal uses of the member or class to either use the new approach that replaces the obsolete approach, or mark that member or class itself as [Obsolete]

Step 3. If you've marked new stuff as [Obsolete] in Step 2, repeat this step as needed.

Step 4. Remove all obsolete members and classes that are neither public nor used by an obsolete public member or class.

Step 5. Update documentation to give a clearer description of the approach recommended to replace any public obsolete members or classes.

At the end of this, you will have no obsolete code that is solely used by internal code. There's nothing to say that you have to do all of this in one go though; at each stage you have made progress. The time between starting step 1 and ending step 5 could be 5 seconds or 5 years, depending on many factors (most of them to do with complexity).

Incidentally, if someone finds it easy to ignore compiler warnings, the problem is not with [Obsolete]. However, one reason not to leave such calls in the code for long (that is, to have done as far as step 2 ASAP) is to make sure people don't end up becoming used to compiler warnings as they're part of the habitual response to compiling the code.

Answer 4

It depends. Yes, you COULD refactor the code. COULD YOU?

The problem is - youCAN refactor WITHIN ONE PROGRAM. It is a lot harder if the API is out in the public and you simply CAN NOT refactor code using your API. This is what Obsolete is made for.

if the API is internal to your code, then refactoring is the way to go. CLean up code, do not leave a mess.

But if the public API changes, it should - if possible - be done slowly.

The rest is still subjective. I do not like "Obsolete" for internal API's.

Answer 5

I've used it before as sort of a temporary state of affairs when we've got old code that needs to be refactored eventually but not urgently. Most often this is the case when some new code has been written that gets the job done better than what came before it, but nobody on the team has the time to go back and replace a lot of old code at the moment. Obviously this implies a situation where a simple drop-in replacement is not immediately possible (sometimes the new code does almost everything the old code did, but there's a small bit of functionality that hasn't been implemented yet).

Then having all those compiler warnings sitting around is a constant reminder for someone to go back and finish up the refactoring when he's got a little bit of free time.

Whether this is really a good or bad thing is pretty subjective. It's a tool, like any other.