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.