Some programming annoyances

I write and maintain mostly C# these days. I've been accumulating a list of annoyances. Whenever I come across a piece of code that makes me groan, fills me with fear or just bothers me, I add it to the list. It helps keep me sane. Some of them are obvious and blatant; nobody should be doing those things! Some of them are subtle. Some of them are debatable. All are related to actual code I've dealt with. Some of them are things I do myself and then feel guilty about. Maybe you'll find them interesting. Maybe you'll disagree.

C#

Most of these are C# or at least .NET-specific. Some are general to object-oriented languages.

Don't lock on public objects

This exposes your implementation and invites deadlock.

Don't lock on String.Empty

Seriously. WTF. I mean, locking on strings is weird and problematic enough, but locking on String.Empty??

Don't catch Exception

There should almost always be a more specific exception to catch. If you're catching Exception you have no idea what kind of bad state your program is in and there's very little you can sensibly do. I'll concede that there may be situations where you need to bundle away the exception to rethrow later on, if you're writing some kind of remoting/scheduling framework, but really ask yourself if you need to be doing that. As soon as you catch that exception, you're making it much harder to debug, by separating the symptom from the cause in time and space.

Don't do work in a constructor

It is a horrible code-smell if you construct an object and then throw away the reference immediately, indicating that it is invoked solely for the side-effects of its constructor. Constructors should not be responsible for stitching the newly created object into an object-graph. You have something that resembles a Klein bottle. I find it hard to express exactly what's so bad about this, but it fills me with trepidation when I see it.

If you need to guarantee that some non-trivial work is done before the object is in a valid state, you should not postpone that work until after the constructor. That's worse! Instead require that it has been completed before the constructor is invoked - the prepared components should be arguments to the constructor. Perhaps you will find that you then need another object to assemble the object-graph. That's okay! Assembling a system is a different and quite unrelated task to the job the system performs! Cars, computers, bridges, houses - none of these engineered objects need to assemble themselves. The same is true of objects in programming.

Avoid "new" outside of factories

Okay, this is possibly a controversial one.

The "new" keyword should be relatively rare in your code. It's okay to new up a List or Dictionary, but you should not be constructing objects all over the codebase. This tightly couples code and makes it difficult to test separate components. It's especially bad to have a huge hierarchy of constructors called by each other. Prefer to confine construction to factories. Note that you don't need one factory per class - often it makes sense for one factory to construct a suite of related object classes.

Avoid downcasting

Sometimes it's the pragmatic choice. Most of the time it creates hidden dependencies and surprising behaviour.

Don't use "indicator" interfaces

Interfaces without members are a bad smell. Don't use interfaces which are just indicators that the object can be used in some other way, such as by casting.

One pattern that does this a lot that I don't have a great answer for, is the "interface that returns handles". Suppose you define an interface for a file-system, and an interface for the files/folders it contains. The file-system hands out instances of the file interface to represent the files it contains. The question is, how do you describe file-system operations that affect two files? Say "move this file into that folder"? How do you design this so that you can have different implementations of the file-system interface with different hidden implementation details, but still have a natural and useful API for users? I've seen a lot of systems that just take IFile as an argument, and fail if the downcast to MyFileType fails or if file belongs to a different file-system. That seems downright nasty, but I most of the alternatives I can think of are still at least a bit unpleasant.

Prefer signed integers

Unsigned integers are unusual in .NET APIs. They're not CLS compliant - other .NET languages might not have them. They also behave badly, silently wrapping when decremented below 0. It's better to take signed integers as parameters and throw an exception when a negative value is provided than to take unsigned integers and fail in weird ways when somebody accidentally decrements beyond 0. Unsigned integers should only be used when it's important to have the entire range of positive values. Also beware not to do this:

for (uint i=9; i>=0; --i) { ... }

This advice is quite specific to C#. It sometimes applies in C++, but unsigned integers are much more pervasive there, particularly size_t, so it's not such practical advice. It doesn't apply at all to languages that don't have silent wrapping.

Dispose in reverse order of creation

Objects with *similar* lifetimes should have *nested* lifetimes. Anything else is horrible to keep track of and maintain. That said, most of the worst cases of this I've seen have involved other bad practices, like massive constructors that call a million other constructors. Nothing is less fun than trying to fix an "object diposed too early" bug and finding that it's one of thirty other objects disposed by its parent in seemingly random order.

Don't have properties that modify state

It's surprising, and it will do weird stuff in the debugger. Property access should not observably modify state. (Caching an unchanging result is probably okay.)

Don't hand out "this" in a constructor

Here's another one I think might be controversial. I'd be interested in feedback on this one, and I'll try to answer questions anyone has.

See also "Don't do work in a constructor". If you pass out references to "this" (including bound methods, or closures that capture "this") you are revealing your partially constructed object and allowing it to be meddled with before it has established its invariants. This will end in tears if you have subclasses. Even if you don't have subclasses, it's hard to maintain. It becomes unclear where in the constructor the invariants have been established, and it breaks easily when someone tries to re-order the code in the constructor.

Don't throw from a Dispose method

It's too late. You don't know if your object is being disposed due to stack unwinding passing through a "using" block, in which case you will trash the useful information in its stack trace and replace it with less useful information from your own. If you're doing something in a dispose method that might fail, prefer to give users a way to do it manually before the object is Disposed, and throw at that point. I'm not 100% certain on this one. There are cases where it's a programmer error to forget to do something before disposing an object and it would be nice to flag it up.

Prefer not to expose IDisposables to clients who are not responsible for disposing them

It's just too confusing otherwise. A corollary is that you should be very careful about declaring interfaces that inherit from IDisposable. Should all clients of your interface have the power to dispose the object?

Avoid null values when possible, and document them clearly where they're necessary

Nulls are insidious, and love to get into places they're not supposed to be. Avoid them in APIs when possible.

Avoid null with the meaning "key not found"

This just invites your clients to make a mistake and forget to check if the value is null. Prefer to throw an exception, or use a TryGetValue approach, or let the client specify what to return when the key is not found. If you must, document it.

Don't repeat yourself

Do I need to say this one? Don't repeat yourself. It just gives you more places for bugs to live and more places you need to remember to fix bugs.

Prefer composition to inheritance

Inheritance exposes part of your implementation. It also tightly couples you to the base class implementation. Prefer composition as a first resort, and inheritance only when it is well-suited to the problem. If you're not going to make use of polymorphism, think very carefully about why you're using inheritance.

Don't hold references "for a friend"

Especially don't hold references solely for the purpose of passing them on to a constructor later on. See also "Avoid 'new' outside of factories".

I'm guilty of this myself. I think this tends to go hand-in-hand with Law of Demeter violations. It's probably a sign that your objects know too much about each other. However, it can take a lot of careful thought to iron out these problems. I have a feeling that a good dependency injection framework is helpful here, but sadly I've not been able to use one much lately, so I've got no first-hand evidence.

Ensure no methods on an object will be called after you enter its Dispose method

Just as a constructor shouldn't hand out references to "this", a Dispose method shouldn't be expected to defend against use of outstanding references to "this". You may choose to detect and flag up this situation, but it should be considered an error, not the intended behaviour. The same problems with sub-classes arise. Clients of the class should guarantee that all other uses of it happen-before invocation of Dispose. If the class itself manages a thread or by other mechanisms causes concurrent invocations throughout its lifetime, *those concurrent invocations should not be to the object itself*.

This goes double for C++ destructors. You are doing something extremely dodgy if any other thread is calling methods on your object after control has entered the destructor.

NUnit

My list was almost entirely general C# stuff, but there were a few items specific to unit-testing.

Avoid Debug.Assert

Use the NUnit asserts. They give better error messages.

Don't Assert.IsTrue(false)

This is perverse. Prefer more specific assertions which can give you more information about what went wrong. E.g. Assert(foobar.Counter, Is.EqualTo(5)) will tell you the actual value of foobar.Counter when it fails. Even if there is no more specific assertion, prefer Assert.Fail, which is clearer as a "we should not have gotten here" indicator than seeing "Expected true: was false".

Don't repeat yourself

Yes, this counts for tests too. Yes, I understand the irony of repeating myself in telling you not to repeat yourself.