Implicit/Explicit Assertions

Question

During code review, a minor suggestion was presented that some implicit behavior be made explicit. The reviewer had skimmed over the code in question, became confused after mistakenly interpreting the code's purpose, and only discovered the cause after some careful analysis. Below are the implicit practice and suggested practice, in that order:

stuffLookup = {--[[functions in here]]}

function doStuff(stuffType, ...)
    assert(stuffLookup[stuffType], "stuffType is not a valid stuff!")(...)
end

function doStuff2(stuffType, ...)
    if stuffLookup[stuffType] then stuffLookup[stuffType](...) else error("stuffType is not a valid stuff") end
end

assert is frequently used without invoking another function as in this case, so at first glance the reviewer did not see such a small invocation at the end of assert, and assumed the line served no purpose past a sanity check.

The argument in favor of practice 1 was that it's syntactically correct, that it's used in Lua docs, that it's faster, and that if someone didn't anticipate assert being used like that it was their own fault. The argument in favor of practice 2 was that the nature of practice 1 could easily be misunderstood, that it was an implicit idiosyncrasy that wasn't legible enough, and that considering performance was premature optimization (code was not designed to be used frequently and table lookup time was negligible).

Are there any merits to the concerns and viewpoints of either party?

Answer 1

This seems primarily opinion based, but I'd suggest option 3:

  function doStuff3(stuffType,...)
    local stuffCallback = assert(stuffLookup[stuffType], "stuffType is not a valid stuff!")
    stuffCallback(...)
  end

The reason for this is that double parenthesis is very often unexpected and thus its use violates the Principle of Least Astonishment. By storing it in a local variable that provides a name for what it is you're doing, you make sure that we, the readers, know that the code is calling a function, so it isn't particularly astonishing to see the parenthesis.

You would maintain the assert, so it should die appropriately, be easily found by searching, shows exactly what you mean to do, and be relatively performant (I'm not an expert on Lua's local variable performance as opposed to transient values, though).

As far as the merits and concerns of each view: Syntactically correct is hardly a viable argument --- if it works at all it's syntactically correct. Something being used in documentation isn't a sign that it is "good", either. Documentation often omits things for the sake of clarity, or succinctness, or does it just because the author thought it was clever. As far as fault, it doesn't matter who's at fault. What matters is that a not-insignificant number of maintainers will have to spend extra time and mental energy on understanding the system that could otherwise be used on enhancing the system. However, practice 2 has its own issues that you haven't mentioned. It's more difficult to search for. It's less succinct. It's less DRY. Reading "assert" tells you exactly that this was meant to be an assertion, whereas reading the if-else makes you spend mental energy on determining that this is a validation conditional. It's also so long it requires scrolling in the question, but this excess horizontal movement is also a possible problem with practice 1. Breaking it up into discreet units keeps the flow of the code vertical, where it's easier to follow.