3

Producing predictable output for each possible input is the responsibility of each module. For example (in C#):

class Logger
{
    public ITextWriter Writer { get; set; }

    private uint counter;

    /// <summary>
    /// Writes message in special format and returns the number of total messages written
    /// </summary>
    public uint Debug(string message)
    {
        if (message == null) throw new ArgumentNullException("message");

        if (Writer == null) throw new InvalidOperationException("Writer not set");

        Writer.Write(string.Format("{0:HHmmss}: [DEBUG] {1}", DateTime.Now, message));

        return ++counter;
    }
}

Logger module is quite verbose, but it's output - exception, call to a depedency and returned value - is predictable and obvious for each argument and state.

But one possible use-case bothers me. What if Writer was set to some weird implementation which somehow calls the Logger.Debug method on that same object? First possible consequence is stackoverflow due to infinite recursion. Second - unpredictable output and possible bugs. It's obvious that such situation must be controlled somehow. If we want predictable output for our logger, it should control double-entries:

    // ...
    private bool enter;

    public uint Debug(string message)
    {
        if (message == null) throw new ArgumentNullException("message");

        if (Writer == null) throw new InvalidOperationException("Writer not set");

        if (enter) throw new InvalidOperationException("Double-entry");

        enter = true;

        Writer.Write(string.Format("{0:HHmmss}: [DEBUG] {1}", DateTime.Now, message));

        enter = false;

        return ++counter;
    }
    // ...

And it seems that every external call (a call not to it's own components) must be wrapped with such flags. That looks crazy!

Is it how good code written? Or is it normal to beleive that your system has no circular calls? Am I missing something important? Please, advise.

UPDATE

It's even worse, guys:

    // ...

    private bool enter;

    public uint Debug(string message)
    {
        if (message == null) throw new ArgumentNullException("message");

        if (Writer == null) throw new InvalidOperationException("Writer not set");

        if (enter) throw new InvalidOperationException("Double-entry");

        try
        {
            enter = true;

            Writer.Write(string.Format("{0:HHmmss}: [DEBUG] {1}", DateTime.Now, message));
        }
        finally
        {
            enter = false;
        }

        return ++counter;
    }
    // ...

I beleive that catching an exception is on duty of higher-level module (the one which created them all), so exceptions must follow their way. But the state after unsuccessfull call must remain correct. Exception is not the end of the world, and modules possibly can be re-used.

  • Just a note public uint Debug(string message) violates the CQS principle. – Songo Sep 10 '14 at 12:58
  • OK, thanks. There must be a separate method for querying the counter. – astef Sep 10 '14 at 13:00
  • 1
    What about if the Writer thrown an exception? You couldn't log it else where since you Debug would throw the Double-entry exception. – Rémi Sep 10 '14 at 13:21
  • Hm... Now I see the need of exception catching around each external call. Even more problems, thank you, @im_a_noob ! – astef Sep 10 '14 at 13:26
  • @im_a_noob This exception must be catched and logged by the one who created Writer and Logger. Possibly, Program class should catch it, write something to the console's error output and exit. See updates – astef Sep 10 '14 at 13:48
3

If you use design by contract then you could specify that implementations of ITextWriter should not call Logger.Debug.

Only if it's obvious that some implementation would have a reason to call Logger.Debug would you bother defending against such a case.

A more interesting question would be a multi-threaded situation. Does the responsibility of handling multi-threaded calls fall to Logger or the implementation of ITextWriter? Or is your library not handling the case of thread-safety? (Currently your implementation of Logger is definitely not thread-safe due to your incrementing of the counter member).

As you can see, your code can only handle the "known knowns" so to speak.

  • It is not necessary that ITextWriter calls Logger.Debug. It can call something else which will call something else... until someone will try to write something to a logger and... oops! – astef Sep 10 '14 at 12:54
  • @astef - if you've identified that use case, then you should do something to handle it. Either specify that it's the responsibility of the ITextWriter implementer not to do that, or leave in the guard clause. – Scott Whitlock Sep 10 '14 at 12:56
  • Yes, but the only way I see to avoid those bugs in early stage of development - is writing such flags around each external call. Is is what you do in your projects (at least when you try to write clean code)? – astef Sep 10 '14 at 12:59
  • 1
    @astef - this only applies if you're writing a library that's going to be released to the public, or to another department within your organization at least. Only the publicly accessible API calls need such scrutiny. If I was writing a library and I determined this was a likely scenario, then yes I would do it, but in most cases you might not even think of it, and you'll just wait for a bug report to come in where someone did something "weird". Then you can release a new version with a fix like this. It won't change the API, so it's not a breaking change, so don't worry about it too much. – Scott Whitlock Sep 10 '14 at 13:10
  • 1
    @astef - I'm sorry for not making myself clear. You definitely code for cases that you "know" about. We know that we have to validate arguments, so yes we validate arguments on public API calls. If we know that we have to guard against re-entrant calls then we do that too, but we have to live in the real world, and sometimes we don't know all the possible cases up front. All of this is based on experience of what's typical, probable and possible. Does your code handle an OutOfMemoryException? (It shouldn't.) How do you guard against Writer blocking and never returning? – Scott Whitlock Sep 10 '14 at 13:51
0

In general you do something sane, and then at some point incorrect use of your library becomes the programmer's problem.

For example for logging you can look at http://logging.apache.org/log4net/index.html. The writer there is customizable by what level the log is writing at. And then you simply don't have your debug output using a writer that is itself using the logging framework to do debugging. Unless you want to, in which case the writer should guard against infinite recursion by looking at the logging level and then deciding whether it wishes to issue a potentially problematic debug statement.

That is a good enough solution. Any writer should either document itself as not suitable for use with debugging level, or should have that guard to not try to debug itself when so used.

0

And it seems that every external call (a call not to it's own components) must be wrapped with such flags. That looks crazy! Is it how good code written? Or is it normal to beleive that your system has no circular > calls? Am I missing something important? Please, advise.

Levelization.

Levelization will be a key concept for a long time to come. The idea is for pieces of code or components to be separated into levels according to what they focus on doing. I suggest the OP think about levels of methods: higher methods will only call lower ones. Conversely, if authors take a shortcut by writing fewer methods that are all peers, they need high-maintenance guard logic and they start to become ludicrously spaghetti- like. Levelization scales well. Future users: search for it

Here's a recent Google result:

http://blog.coverity.com/2014/01/24/software-complexity-and-levelization/

  • Can you summarize the article? Links go bad all the time & "Link only" answers are not well liked here. – Dan Pichelman Sep 10 '14 at 17:39
  • Levelization will be a key concept for a long time to come. The idea is for pieces of code or components to be separated into levels according to what they focus on doing. I suggest the OP think about levels of methods: higher methods will only call lower ones. Conversely, if authors take a shortcut by writing fewer methods that are all peers, they need high-maintenance guard logic and they start to become ludicrously spaghetti- like. Levelization scales well. Future users: search for it. – geac Sep 17 '14 at 14:21
0

If this is something you're really worried about, then it's clearly a cross-cutting concern, so it's one you could tackle with an Aspect-Oriented Programming approach.

For example, you could use Castle's DynamicProxy, and write an interceptor that keeps track of the entry variable and throws an exception if the same method is entered a second time. If you need more granular behaviour, like some methods being allowed to recurse or having a maximum number of allowed entries greater than one, you could control that with attributes. Obviously this approach will still only give you exceptions rather than design-time safety.

Generally, however, I'd say that this concern is outside what a module should take worry about. For example, let's say I have the following method:

public void CreateItem()
{
    _itemStorer.Create();
}

Doesn't get much simpler than that. But what if when this is called, the stack is already almost full (not necessarily because of any recursion), and any calls into further methods will cause a stack overflow? Or what if the stack is half full, but _itemStorer puts enough items on it with Create to overflow it? Should this simple method take responsibility for knowing the stack usage of all of everything above and below it? Surely that way lies madness.

Or, an even simpler version, what if the day after you write this piece of code, somebody comes in and replaces all of the implementation of Create with throw new NotImplementedException()? Should you expect to be able to handle that too?

A module of code should take responsibility for itself, which means behaving and interacting with its dependencies in a safe and sane way. It should not be expected to take responsibility for its dependencies. Remember, if A takes B as a dependency, then B is being used to fulfill a need that A has, and it is B's responsibility to do that correctly.

  • StackOverflowException,OutOfMemoryException,... will be thrown not in my code, it is not my responsibility to catch them. My responsibility is to keep my module's output predictable on every possible input. From the perspective of A, it's function double-entry is always possible if control was given to B. Even if B has no bugs. The bug can occur in a higher level - in a module which uses them wrong. So, if A has no predictable output on each possible input - you just don't know what will happen if you'll use your components wrong. – astef Sep 18 '14 at 8:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.