General function question (C++ / Java / C#)

Question

This question is probably language-agnostic, but I'll focus on the specified languages.

While working with some legacy code, I often saw examples of the functions, which (to my mind, obviously) are doing too much work inside them. I'm talking not about 5000 LoC monsters, but about functions, which implement prerequisity checks inside them.

Here is a small example:

void WorriedFunction(...) {
   // Of course, this is a bit exaggerated, but I guess this helps
   // to understand the idea.
   if (argument1 != null) return;
   if (argument2 + argument3 < 0) return;
   if (stateManager.currentlyDrawing()) return;

   // Actual function implementation starts here.

   // do_what_the_function_is_used_for
}

Now, when this kind of function is called, the caller doesn't have to worry about all that prerequisities to be fulfilled and one can simply say:

// Call the function.
WorriedFunction(...);

---

Now - how should one deal with the following problem?

Like, generally speaking - should this function only do what it is asked for and move the "prerequisity checks" to the caller side:

if (argument1 != null && argument2 + argument3 < 0 && ...) {
   // Now all the checks inside can be removed.
   NotWorriedFunction();
}

Or - should it simply throw exceptions per every prerequisity mismatch?

if (argument1 != null) throw NullArgumentException;

---

I'm not sure this problem can be generalized, but still I want to here your thoughts about this - probably there is something I can rethink.

If you have alternative solutions, feel free to tell me about them :)

Thank you.

Lipolysis

While language specific, I think msdn.microsoft.com/en-us/devlabs/dd491992 might be of interesting to you. – Wolf

Someday, when IDEs are a bit more visual (like Code Bubbles), I think these design contracts will be something you can just specify by double-clicking the method and inputting them into a text-box (or even better, choosing them from a drop-down menu). Not only will they then no longer clutter the code (as they really shouldn't), but also the documentation generation will be automatic! – Blackguard

Answer 1

Every function/method/code block should have a precondition, which are the precise circumstances under which it is designed to work, and a postcondition, the state of the world when the function returns. These help your fellow programmers understand your intentions.

By definition, the code is not expected to work if the precondition is false, and is considered buggy if the postcondition is false.

Whether you write these down in your head, on paper in a design document, in comments, or in actual code checks is sort of a matter of taste.

But a practical issue, long-term, life is easier if you code the precondition and post-condition as explicit checks. If you code such checks, because the code is not expected to work with a false precondition, or is buggy with a false postcondition, the pre- and post- condition checks should cause the program to report an error in a way that makes it easy to discover the point of failure. What code should NOT do is simply "return" having done nothing, as your example shows, as this implies that it has somehow executed correctly. (Code may of course be defined to exit having done nothing, but if that's the case, the pre- and post- conditions should reflect this.)

You can obviously write such checks with an if statement (your example comes dangerously close):

if (!precondition) die("Precondition failure in WorriedFunction"); // die never comes back

But often the presence of a pre- or post-condition is indicated in the code by defining a special function/macro/statement... for the language called an assertion, and such special construct typically causes a program abort and backtrace when the assertion is false.

The way the code should have been written is as follows:

void WorriedFunction(...)  
 {    assert(argument1 != null); // fail/abort if false [I think your example had the test backwards]
      assert(argument2 + argument3 >= 0);
      assert(!stateManager.currentlyDrawing());
      /* body of function goes here */ 
 }  

Sophisticated functions may be willing to tell their callers that some condition has failed. That's the real purpose of exceptions. If exceptions are present, technically the postcondition should say something to the effect of "the function may exit with an exception under condition xyz".

Eteocles

+1, but you probably want ! instead of ~ in the last assert (~ in C++, C# and Java is the bitwise not operator) – Impossibility

@dave: picky, picky... he said any generic language :-} I changed it to make you happy. – Eteocles

Yeah but the question is tagged C++, C# and Java :P – Impossibility

Answer 2

That's an interesting question. Check the concept of "Design By Contract", you may find it helpful.

Battaglia

Answer 3

It depends.

I'd like to seperate my answer between case 1, 3 and case 2.

case 1, 3

If you can safely recover from an argument problem, don't throw exceptions. A good example are the TryParse methods - if the input is wrongly formatted, they simply return false. Another example (for the exception) are all LINQ methods - they throw if the source is null or one of the mandatory Func<> are null. However, if they accept a custom IEqualityComparer<T> or IComparer<T>, they don't throw, and simply use the default implementation by EqualityComparer<T>.Default or Comparer<T>.Default. It all depends on the context of usage of the argument and if you can safely recover from it.

case 2

I'd only use this way if the code is in an infrastructure like environment. Recently, I started reimplementing the LINQ stack, and you have to implement a couple of interfaces - those implementations will never be used outside my own classes and methods, so you can make assumptions inside them - the outside will always access them via the interface and can't create them on their own.

If you make that assumption for API methods, your code will throw all sorts of exceptions on wrong input, and the user doesn't have a clue what is happening as he doesn't know the inside of your method.

Conall

Answer 4

"Or - should it simply throw exceptions per every prerequisity mismatch?"

Yes.

Rearrange

I don't fully agree with that. There are noteable exceptions for that, for example the TryParse methods. – Conall

@Femaref: In case of TryParse, "the input is parseable" is specifically not a prerequisite. – Hopple

I agree, though I'm not going to give my upvote to an answer which you clearly put no effort into writing. Perhaps you could grace him with an explanation as to why this is considered best practice (and why it's better not to force the caller to verify)? – Blackguard

@Femaref: As the name suggests, TryParse tries to do something and tells you whether it succeeds or not. It doesnot tell what exceptions it handled internally. For example let's try Int32.TryParse. This method returns a 32-bit signed integer value equivalent to the number passed as parameter, if the conversion succeeded, or zero if the conversion failed. The conversion fails if the parameter is null, is not of the correct format, or represents a number less than MinValue or greater than MaxValue. So this is a 'specially handled' method and not a general case. – Behistun

Yes, I got it, I missed the prerequisity in the text. I just returned from writing my text which handles the general case of when to throw an exception and made the comment from that assumption. – Conall

@ukhardy: Actually Int32.TryParse returns a boolean :) – Blackguard

I disagree with this. If something truly is a prerequisite, then it should never be violated (if it is, you need to fix the calling code). Exceptions aren't for things that should never happen, assertions are. It may make sense to throw an exception instead of assert if this is a public API however. – Impossibility

@BlueRaja - Danny Pflughoeft: Oh dear! I'm afraid you skipped the first line of my comment... – Behistun

Answer 5

You should do checks before calling and function and if you own the function, you should make it throw exceptions if arguments passed are not as expected.

In your calling code these exceptions should be handled. Ofcourse arguments passed should be verified before the call.

Behistun