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

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 cer开发者_开发百科tain 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.

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.

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.

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.

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.