Should code prevent a logically invalid call even when no harm would be done?

Question

This one has been puzzling my for some time now.

Let's imagine a class which represents a resource, and in order to be able to use this resource one needs to first call the 'Open' method on it, or an InvalidOperationException will be thrown.

Should my code also check whether someone tries to open an already open resource, or close an already closed one?

Should code prevent a logically invalid invocation even when no harm would be done?

I think that programming this way would help writing better code at the other side, but I feel that I might be taking too much responsibility and affect reusability.

What do you guys think?

Edit:

I don't think this could be called defensive programming because it won't let a possible bad use to slip either, and another InvalidOperationException will be thrown.

Answer 1

This is called defensive programming. That's a good programming practice because you ensure that your application doesn't crash on misbehaviour.

That some method should be called first before another method is called, is not a good programming practice. It add's a lot of complexity, which is better handled by the class itself.

This is called sequential coupling. This wikipedia article says that it depends on the context if it's a bad practice, but it shouldn't crash when handled improperly. Sometimes it's necessary to throw an exception to make things clear.

Answer 2

If your example is really the case, then the Open functionality should probably be invoked by the class's constructor.

If you consider the C++ iostream library (which is very widely used and considered quite a good example) you can call any operation on a stream class, whether it is open or not. The called function will simply return a failure indicator of some sort if the operation could not be performed. The functions must of course test the stream state in order to do this.

What you must not do is allow your programs to silently accept any old input as parameters. For example, this would be a broken implementation of strlen()

int strlen( const char * s )
{
   if ( s == 0 )
   {
      return 0;     // bad
   }
   else
   {
      // calculate length not shown
   }
}

as it fields bad inputs without causing a fuss - it should instead throw an exception or use an assert(), depending on your exact development philosophy.

Answer 3

There's no substitute for taste, talent and experience in figuring out exactly how many safety checks should be in your code for best cost/benefit ratio for your organization.

A good quality APIs are expected to be fool-proof, and to guide the user with proper amount of warnings.

Sometimes, safety precautions may impair performance. Performance is one of the most counter-intuitive things in programming. Optimize with care, only when performance really matters.

Answer 4

This really depends on what the class actually does. In some cases failing silently is a good idea (eg, you want your DVD player to continue working, not show an error message if it opens the DVD tray that is already open) and in other cases you want as much information as possible (eg, if an airplane tries to close a door that is reportedly already closed, then something is wrong and the pilot should be alerted).

In most cases throwing an error when a logically invalid action is performed is useful for developers, so implementing those exceptions depends on who will use the code. If it is used internally for one application, then it's not vital. But if it is used by many different projects or developers, then I would look into it.

Answer 5

If this is part of a public SDK that you're releasing to the wild, then the exposed API calls should have strong validation. It will help your 'users' (who are developers) and ensure you aren't stuck supporting usage you never intended to support.

Otherwise, I would not add such checks. I think they make the code harder to read, and these checks are rarely tested. In the past I would add a lot of code like this to make sure my code doesn't do the wrong thing. Now I write unit tests to verify my code does the right thing. The difference? I think tests are more maintainable, more readable, and they don't clutter your production code.

Answer 6

In the case of opening a file that is already open, it depends on knowing the effect of the request, will it reset the current read location for example.

In the case of closing a file that is already closed, think of it as a request for the file to be put in a known state. The code doesn't have to do anything but the desired state is acheived so the code can return a success condition. This is not true if there is some sort of file buffering that needs to be taken care of or maybe an interlinked resource to coordinate, like a modem/serial port or a printer/spooler.

Step back and think of the problem in terms of the desired outcome including any side-effects.

We once put a 'logout' link on an app menu that was displayed regardless of your login status. Why? Because it only took a simple (and very short) method to handle returning you to the login screen from the login screen and saved a large number of checks to handled tracking the login status just so the 'logout' menu-item was displayed only when you were logged in.

Answer 7

logical invalid invocations should always be reported to the user in debug mode..

When compiled in release mode, your code should not throw any unneeded exceptions or do anything else which could endanger the whole application. Personally i prefer having some kind of logfile, and logging such logically invalid invocations surely will do no harm (at least when performance is not important)