24

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?

g t
  • 7,287
  • 7
  • 50
  • 85
  • 3
    Sounds like a reason to force warnings to be errors – Matt Ellen Aug 18 '10 at 13:02
  • @Matt - True; we've now made this change to prevent [Obsolete] being used in future, amongst other things – g t Aug 19 '10 at 14:41
  • 4
    There's nothing wrong with using `[Obsolete]` in this case. Just because you've created a better widget doesn't mean you have the time to go through and rip out all the places where the bad widget is used. At least by marking it obsolete you've indicated that people shouldn't use it in the future and remove it when possible. – Michael Richardson Apr 09 '14 at 20:28

5 Answers5

28

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.

Jon Hanna
  • 110,372
  • 10
  • 146
  • 251
7

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.

flq
  • 22,247
  • 8
  • 55
  • 77
  • if you introducing [Obsolete] attribute you have to fix all resulting compilation warnings by yourself. Otherwise, you just littering your own source code. – Boris Lipschitz Mar 30 '11 at 06:56
  • 3
    Did you actually read what I wrote. It is a temporary measure for a larger refactoring. And then you downvote? Ts... – flq Mar 30 '11 at 11:20
  • I didn't like "and whoever had time performed refactorings until all calls were gone". I see it sometimes in large projects. Some people put obsolete attributes and fix there main area of concern. Then wait for someone else "who has time" to fix other usages. – Boris Lipschitz Mar 30 '11 at 22:23
6

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.

TomTom
  • 61,059
  • 10
  • 88
  • 148
  • +1 Sensible. Tools (ReSharper) exist that can systematically refactor internal code that depends on an API, and if you have unit test coverage then such refactorings should be safe. (If you don't have unit test coverage, then should you really be changing this code in the first place?) – Tim Robinson Aug 18 '10 at 10:12
  • Thanks for your thoughts - someone else who thinks of this attribute as a 'mess' when used internally. (Note the code in question wasn't accessed via an API, it was on an internal library.) – g t Aug 18 '10 at 10:16
  • I would definitely refactor. Point is - it eliminates code. Double code = expensive ;) – TomTom Aug 18 '10 at 10:18
  • @Tim Robinson. Sadly, we're not allowed/trusted with ReSharper. It certainly would have helped in this case as you say. There were unit tests associated too, although when dealing with encryption it can make it harder to be certain every case been fully covered by tests. – g t Aug 18 '10 at 10:19
  • Shame - ReSharper has made me a much better developer. – Tim Robinson Aug 18 '10 at 10:22
4

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.

Dan Tao
  • 125,917
  • 54
  • 300
  • 447
  • Quite! The attribute seemed to serve as a reminder that this code should be considered for removal in future without having to make that decision when approaching a release. – g t Aug 18 '10 at 10:21
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.

Ben Robinson
  • 21,601
  • 5
  • 62
  • 79