Am I using a global state here, is there any better way to do this?

Question

I am modifying some legacy code. I have an Object which has a method, lets say doSomething(). This method throws an exception when a particular assertion fails. But due to new requirement, on certain scenarios it is okay to not throw the exception and proceed with the method.

Now I am not calling this method directly from the place where I need to ignore the exception. This doSomething() is like an audit method which is called internally from many other methods, lets say method1(), method2(), etc.

In the place where I need to ignore the exception, I am calling method1(), now I do not want method1() to throw the exception. So I modified method1() to take a default argument method1(ignoreException = false) and called method1(true).

I also modified doSomething() to take the extra argument and method1 just passes the ignoreException back to doSomething(ignoreException).

Potentially, I need to change all the methods, method2, method3 etc as well to take this extra argument.

On seeing this code, someone suggested that instead of passing this flag around, I can have it as a member variable of the class and then call the setter before calling method1(). Lets say my object is obj, then I should do obj.setIgnoreXXXException(true); obj.method1(); obj.setIgnoreXXXException(false);

This seems to me like maintaining some global state and doesnt seem right. But the other way of passing around arguments also seem to be clumsy and I have to change a lot of places (this class has subclasses and some methods are virtual so I need to modify everywhere)

Is there a better way of doing this. Since it is legacy and there are no unit tests, I do not want to modify a lot of existing code.

Answer 1

Specify a static boolean variable on the class, and a static member on that class that allows you to set the boolean to whatever value you choose; you can use that static boolean to suppress the throwing of exceptions, and can set it from your code without needing to modify any existing interfaces.

Answer 2

You could go the route of creating some sort of wrapper (either a class or a functional closure). Then, you could define a variable which is a use of the class you've already got. It would wrap the use of the class, so you could avoid some of the management steps.

var wrapper = yourClass.WrapWithThrowOption(true);
wrapper.method();

or

var wrapper = wrapWithOption(true, method1);
wrapper();

You didn't indicate which language you're using, so I just used some pseudo-syntax.

Answer 3

You certainly should be doing this with a function argument, not a member - the choice of whether to ignore the checks is a property of the function invocation, not of the object.

Using persistent state to hold a temporary condition will give you two main problems:

• exception safety - if the function throws an unhandled exception, then your code will leave the "ignore" flag set.
• reentrancy - calling the function recursively, or from multiple threads, may have unexpected results

You can make it exception safe by using a destructor to reset the flag:

class IgnoreException
{
public:
    explicit IgnoreException(Object &o) : object(o)
    {
        object.setIgnoreException(true);
    }
    ~IgnoreException()
    {
        object.setIgnoreException(false);
    }
private:
    Object &object;
};

void callMethodOneIgnoringException(Object &object)
{
    IgnoreException ignore(object);
    object.method1();

    // the flag is restored here, even if an exception was thrown.
}

You can't make it reentrant. Any function that accesses persistent state is not reentrant, so the only fix is to use a function argument.

Answer 4

I'd also recommend using a function parameter instead of a class variable.

However, I generally recommand using an enum instead of a bool:

method1(true);
// true means do throw an exception?
// do supress an exception?

enum ExceptionSuppressionType
{
  SUPPRESS_NO_EXCEPTIONS,
  SUPPRESS_ALL_EXCEPTIONS
};
method1(SUPPRESS_ALL_EXCEPTIONS);
// I'm pretty sure this will suppress the exceptions.

This also gives you a lot more freedom in the future if you decide you want some variation... maybe only suppress some exceptions, or rethrow them in a new form, etc. Booleans are great at reprenting something that can only ever have two choices, but the range of return values and parameters often grow over time.