Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Are assert methods acceptable in production?

At work, I ran upon this method and it troubled me. The fact that the name of the method, the documentation and the implementation don't really fit each other aside (and the pass true to make the condition on true become a false), I don't get the point in having such a method (this is production code):

# @brief An utility method that raises if the singleton is not in a good
# state
#@param expect_instanciated bool : if True we expect that the class is
# allready instanciated, else not
# @throw RuntimeError
@classmethod
def singleton_assert(cls, expect_instanciated=False):
    if expect_instanciated:
        if not cls.started():
            raise RuntimeError("The Settings class is not started yet")
    else:
        if cls.started():
            raise RuntimeError("The Settings class is already started")

Here are my concerns :

  • this "assert" function doesn't tell me much... And would let the client think it will always return a boolean.
  • "None" is meaningless, no information provided to the client, never. The client can't save or pass that information, or would have to write few lines to simply set a boolean variable ;
  • I think exceptions ought to be thrown when the method can't do its work properly, for unexpected behavior. Here it does, clearly !
  • this method is constraining the client to handle an exception he may not care about, and doesn't give him it's own choice without using a try..catch block, and this seems not fine to me ;
  • those raised exceptions seems like pretending to be a normal return-value, it's like if a "file_exists()" method raised exceptions when it there's no file, None when it does ;
  • to me, leaving this kind of method in production means I am not confident on the stability or design of my code. There shouldn't be more than one singleton instance. For instance, python's exists() method is not an assertion, and always return a boolean ! I don't see any reason for letting this method slip into production as it is (and should be actually removed, or moved to a unit test suite).

I am pretty confident that this method simply is wrong. However, knowing if this particular code is okay or not is one thing, knowing if this would be the case in a more general way is another. Thus:

Is it okay for an assert method to be put in production ?

EDIT : as commentators pointed out, it is a rather usual assert method (as there is in the python core code).

However, doing deeper researchers target at this type of method, I strictly ran upon "unit testing" or "debugging" context, with some claiming that it should not be pushed into production. I wouldn't have bugged on this piece if it was in a testing/debugging context.

Thus: any more clarification on the viability of letting assert method slip in production environment ?

EDIT 2: When should assertions stay in production code? This is one of the only talks I founded while Googling. Not clear to me yet.

like image 400
Shirraz Avatar asked Mar 16 '17 14:03

Shirraz


1 Answers

This method is fine.

this "assert" function doesn't tell me much... And would let the client think it will always return a boolean.

assert is a common idiom in many languages. The purpose is to check conditions that are expected to always be true. A failed assertion indicates a coding bug. Therefore, the method returns nothing normally, and throws an exception if the condition fails.

"None" is meaningless, no information provided to the client, never. The client can't save or pass that information, or would have to write few lines to simply set a boolean variable ;

There's no need to check the result of an assertion.

I think exceptions ought to be thrown when the method can't do its work properly, for unexpected behavior. Here it does, clearly !

Assertions are supposed to pass 100% of the time. An exception is only thrown when the condition unexpectedly fails.

this method is constraining the client to handle an exception he may not care about, and doesn't give him it's own choice without using a try..catch block, and this seems not fine to me ;

Assertion exceptions should never be handled. They indicate a coding error. There is no expectation that programs should recover from bugs. It is usually best to simply crash.

I'm making a distinction between bugs and other exceptional circumstances. An I/O error like file not found is a user or system error that you need to plan for. You should catch those exceptions and recover from them. A null pointer exception or an assertion error are signs of bugs. You can't recover from them because when they happen who knows what else might have gone wrong? Bugs need to be fixed, not handled.

However, doing deeper researchers target at this type of method, I strictly ran upon "unit testing" or "debugging" context, with some claiming that it should not be pushed into production. I wouldn't have bugged on this piece if it was in a testing/debugging context.

There's nothing wrong with running assertions in production code. They're not harmful, or bad design. They're good design, in fact. They confirm that the system is running correctly.

The reason people sometimes disable assertions in production code is for performance. They don't want the overhead of extra checks. Personally, I don't work on any programs that are that performance-sensitive, so I leave assertions enabled at all times. The extra safety is well worth the negligible cost of a few if checks.

like image 71
John Kugelman Avatar answered Sep 30 '22 19:09

John Kugelman