7

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.

Community
  • 1
  • 1
Shirraz
  • 172
  • 1
  • 4
  • 10
  • 5
    Usually an `assert` does not return a boolean, but raises an error if the condition is not met. – Willem Van Onsem Mar 16 '17 at 14:36
  • 1
    Python's own `assert` statement either raises an `AssertionError` exception or doesn't depending on the truthiness of the expression it's given—it doesn't return anything. Therefore I think this function, which either raises an exception or just returns is a reasonable approximation (since one generally can't create new kind of statement in Python). – martineau Mar 16 '17 at 15:04
  • Should't this qustion be closed? It seems to be about opinion and style.And it is also unclear what is being asked, because some of the parameters and details mentioned are not well defined. It may look like a good question from the title but when you read it it is not. – mit Oct 19 '20 at 19:47

1 Answers1

8

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.

John Kugelman
  • 349,597
  • 67
  • 533
  • 578
  • The guy that wrote this method actually is a "optimization first / every other dev sucks / assembly fanatic" head... To the point that he never wants to use third-party library or even rewrote an import mechanism for "eliminating any overhead" purpose... Changing my mind is okay, but it would leave me with a "why" you could never answer unfortunately: "why did HE wrote an assert method?" :D – Shirraz Mar 16 '17 at 15:38
  • More seriously. I get your point, still I am not totally convinced. As you rightfully said "it's supposed to pass 100% of time". At this time, I admit this is "fine code", but that it is not for production, as it may hide something wrong (design, bad habits, etc.)... Or that it's a "all or nothing" case: defensive coding with assertions that live in lots of production code parts, or no asserts at all. Probably here is where judgment and personal point of view comes in, but I'll think about it more deeply. – Shirraz Mar 16 '17 at 15:53
  • If something really needs extra safety during run-time you should explicitly state that as an `if ... raise`, otherwise assertions should be considered as useful only for type checkers / tests and not part of prod code (other than for satisfying mypy checks and the likes)... why? because if someone happens to run python with `-O`, then all your asserts are disabled making them the equivalent of no safety. – NotoriousPyro Jul 24 '23 at 08:55