5 ms·

Assert() in the hands of bad coders

18 points by deafcalculus 10y ago

onion2k 10y ago
This use of assert is silly. The code should look like this: In examples 2, 3 and 4 the author explains what's wrong with the code and why it's a bad use of assert(). In example 1 though it's just called silly. What's wrong with that one?
- BoorishBears 10y ago
  The style used doesn't make sense, and defeats the usefulness of an assert error message. You can read an asset in the form I assert that _ will never be true The style shown is saying I assert that false will never be true And the message for the assert failing will be in a similar format
  - unwind 10y ago
    I think you meant "... will never be false", that's what makes the assert(false) so silly. I feel that some programmers don't understand the actual semantic meaning of the word "assert", they just use it as an opaque key word meaning "check this", or something. To me, it should be possible to read an assert almost like English, i.e. assert(handle != INVALID_HANDLE); can be read like "assert that the handle isn't invalid", i.e. is valid. Hm. I'm not stating this very clearly, sorry.
    - BoorishBears 10y ago
      Indeed I meant false (actually "will always be true" would probably have been easier to understand as well)
- CJefferson 10y ago
  I agree, Example 1 is a little weird, but i wouldn't call it wrong. Example 2 is just bad. In example 3 some people compile with exceptions turned off. Asserting on new isn't a problem. Also, "The assert is supposed to check for bad code, not check errors" -- well new returning NULL would be bad code :) Also, in general, if malloc fails asserting is usually your best option, it's basically impossible without heroic efforts to recover from malloc failing, as almost anything you might do in recovery might try another mallocing! Also, example 4 is (in my opinion) fine. I often 'assert' in programs when I have no idea what else to do. Asserting is certainly better than the alternative of just carrying on in a bad state, and fixing up the problem might be hard (I notice there is no suggested "fix" here). Also, many of these comments related back to the original code by 'Satoshi', which makes the final comment, about 1x and 10x programmers hilarious: Yes, of course Satoshi was a '1x' programmer, how date we let them near the bitcoin source code, imagine how much better it would have been without them...
  - cillian64 10y ago
    The idea is that assert is for helping developers catch mistakes, not as a way of dealing with errors when users run the program. Malloc failing or input being invalid are not (necessarily) signs of bugs in the program, and so should not be handled using assertions. Even if the handling is just ending the program, that should be done using exceptions or "return -1", not by asserting false. Asserts and other error handling (exceptions) have different semantics. Disabling asserts says "I believe this code has no bugs", not so unreasonable for a stable release. Disabling exceptions says "I believe nothing unexpected will ever happen", which is several notches crazier.
    - CJefferson 10y ago
      I think we just disagree on what asserts are for. I consider asserts to mean "If this isn't true, there is no way to continue. Something has gone horribly wrong. Just Stop. Now." I suppose I could make up a new function, called say 'check', which basically did exactly the same thing as assert. But why bother? assert is already there and does what I want.
      - dllthomas 10y ago
        Because it's context dependant that assert does what you want. If there's no way to continue, you should also be ending the program in production builds, probably with a prettier error message.
- jasode 10y ago
  >In example 1 though it's just called silly. What's wrong with that one? It's arguably a point about style. The original code saved the effort of typing "assert()" twice but the granularity of exactly which condition failed the assertion is lost. E.g., a MSVC error will show granularity of line# but not the subexpression[1]. So there's a benefit of improved error reporting if the programmer wraps each conditional with its own assert(). [1] https://www.google.com/search?q=c%2B%2B+assert+error&source=lnms&tbm=isch https://www.google.com/search?q=c%2B%2B+assert+error&source=...
xutopia 10y ago
The nuance of calling people names is lost on the author. Bad code can be written by great coders. It happens to the best of us. No need to insult people to improve code.
- markwaldron 10y ago
  I second this. There are plenty of times I'll be looking at code I wrote a few months ago and swear it was written by someone else. The same goes for more elegant code. Not everyone writes amazing code all the time.
- attractivechaos 10y ago
  I second, too. Also: > More generally, though, it shows why there's a difference between 1x and 10x programmers. While I agree that some of the examples are impropriate, I think OP has generalized the issue too far. I don't believe knowing how to use assert properly has much contribution to productivity in programming.
  - imron 10y ago
    > I don't believe knowing how to use assert properly has much contribution to productivity in programming. It's a symptom of the problem, not the cause. It demonstrates that they don't really understand the code they are writing, and it's that attitude and approach to programming that has a contribution to productivity.
    - attractivechaos 10y ago
      This comes back to xutopia's comment. Almost every programmer has blind spots. It is often biased to judge the overall capability of a programmer just by looking at these minor spots. If a programmer is particularly good at breaking a complex problem down to manageable subtasks but knows little about assert, he/she could still be a 10x programmer. The thing is: it is easy to evaluate the proper use of assert, but it is much harder to evaluate the capability to solve complex real-world problems.
    - 10y ago
michaelt 10y ago
Normally, asserts are only compiled into debug versions of the code, and removed for release versions. I know a lot of languages default to this, and it always seemed like a bad decision to me. The performance benefits of most checks are trivial compared to the major downside of not testing the exact code you release. Several times I've had to track down bugs that have been very difficult to find, as they were 'fixed' by side effects of assert statements and hence didn't show up in debug builds. If a check really can't be performed in the release version for performance reasons, just separate it out into a unit test.
- minipci1321 10y ago
  First part of your message seem to be in contradiction with the second one, but anyways -- keeping asserts in the release image and incurring the penalty of added checks (execution speed + all these text strings stored in the read-only segment) does not make sense without the release-approving validation providing the exact coverage of all tested code. That can be tough to obtain.
- jasode 10y ago
  > bugs that have been very difficult to find, as they were 'fixed' by side effects of assert statements and hence didn't show up in debug builds. In those cases, the assert() was used incorrectly. However, that's not a reason to include the assert code in release builds. In a similar way, source code will often have troubleshooting code such as: #if _DEBUG // or #ifndef NDEBUG std::cerr << "inspect size: " << invoices.size() << std::endl; // more debugging-related code ... #endif Just because some programmers mistakenly put unintended side-effects in between the #if_DEBUG/#endif doesn't mean we want that code in the release builds. The purpose of assert() and _DEBUG is to not include them in release builds.
  - marcosdumay 10y ago
    You can't change your program without inserting some kind of side effect. It may be timing in a multi thread system, extra memory available for an out of bounds array read, or even the CPU being kept busy and heating up or avoiding yielding to some other program.
wruza 10y ago
The rule is simple: you never assert() things that can happen, you assert ones that can not.

18 points by deafcalculus 10y ago

This use of assert is silly. The code should look like this: In examples 2, 3 and 4 the author explains what's wrong with the code and why it's a bad use of assert(). In example 1 though it's just called silly. What's wrong with that one?

BoorishBears 10y ago
The style used doesn't make sense, and defeats the usefulness of an assert error message. You can read an asset in the form I assert that _ will never be true The style shown is saying I assert that false will never be true And the message for the assert failing will be in a similar format
- unwind 10y ago
  I think you meant "... will never be false", that's what makes the assert(false) so silly. I feel that some programmers don't understand the actual semantic meaning of the word "assert", they just use it as an opaque key word meaning "check this", or something. To me, it should be possible to read an assert almost like English, i.e. assert(handle != INVALID_HANDLE); can be read like "assert that the handle isn't invalid", i.e. is valid. Hm. I'm not stating this very clearly, sorry.
  - BoorishBears 10y ago
    Indeed I meant false (actually "will always be true" would probably have been easier to understand as well)
CJefferson 10y ago
I agree, Example 1 is a little weird, but i wouldn't call it wrong. Example 2 is just bad. In example 3 some people compile with exceptions turned off. Asserting on new isn't a problem. Also, "The assert is supposed to check for bad code, not check errors" -- well new returning NULL would be bad code :) Also, in general, if malloc fails asserting is usually your best option, it's basically impossible without heroic efforts to recover from malloc failing, as almost anything you might do in recovery might try another mallocing! Also, example 4 is (in my opinion) fine. I often 'assert' in programs when I have no idea what else to do. Asserting is certainly better than the alternative of just carrying on in a bad state, and fixing up the problem might be hard (I notice there is no suggested "fix" here). Also, many of these comments related back to the original code by 'Satoshi', which makes the final comment, about 1x and 10x programmers hilarious: Yes, of course Satoshi was a '1x' programmer, how date we let them near the bitcoin source code, imagine how much better it would have been without them...
- cillian64 10y ago
  The idea is that assert is for helping developers catch mistakes, not as a way of dealing with errors when users run the program. Malloc failing or input being invalid are not (necessarily) signs of bugs in the program, and so should not be handled using assertions. Even if the handling is just ending the program, that should be done using exceptions or "return -1", not by asserting false. Asserts and other error handling (exceptions) have different semantics. Disabling asserts says "I believe this code has no bugs", not so unreasonable for a stable release. Disabling exceptions says "I believe nothing unexpected will ever happen", which is several notches crazier.
  - CJefferson 10y ago
    I think we just disagree on what asserts are for. I consider asserts to mean "If this isn't true, there is no way to continue. Something has gone horribly wrong. Just Stop. Now." I suppose I could make up a new function, called say 'check', which basically did exactly the same thing as assert. But why bother? assert is already there and does what I want.
    - dllthomas 10y ago
      Because it's context dependant that assert does what you want. If there's no way to continue, you should also be ending the program in production builds, probably with a prettier error message.
jasode 10y ago
>In example 1 though it's just called silly. What's wrong with that one? It's arguably a point about style. The original code saved the effort of typing "assert()" twice but the granularity of exactly which condition failed the assertion is lost. E.g., a MSVC error will show granularity of line# but not the subexpression[1]. So there's a benefit of improved error reporting if the programmer wraps each conditional with its own assert(). [1] https://www.google.com/search?q=c%2B%2B+assert+error&source=lnms&tbm=isch https://www.google.com/search?q=c%2B%2B+assert+error&source=...

xutopia 10y ago

The nuance of calling people names is lost on the author. Bad code can be written by great coders. It happens to the best of us. No need to insult people to improve code.

markwaldron 10y ago
I second this. There are plenty of times I'll be looking at code I wrote a few months ago and swear it was written by someone else. The same goes for more elegant code. Not everyone writes amazing code all the time.
attractivechaos 10y ago
I second, too. Also: > More generally, though, it shows why there's a difference between 1x and 10x programmers. While I agree that some of the examples are impropriate, I think OP has generalized the issue too far. I don't believe knowing how to use assert properly has much contribution to productivity in programming.
- imron 10y ago
  > I don't believe knowing how to use assert properly has much contribution to productivity in programming. It's a symptom of the problem, not the cause. It demonstrates that they don't really understand the code they are writing, and it's that attitude and approach to programming that has a contribution to productivity.
  - attractivechaos 10y ago
    This comes back to xutopia's comment. Almost every programmer has blind spots. It is often biased to judge the overall capability of a programmer just by looking at these minor spots. If a programmer is particularly good at breaking a complex problem down to manageable subtasks but knows little about assert, he/she could still be a 10x programmer. The thing is: it is easy to evaluate the proper use of assert, but it is much harder to evaluate the capability to solve complex real-world problems.
  - 10y ago

michaelt 10y ago

Normally, asserts are only compiled into debug versions of the code, and removed for release versions. I know a lot of languages default to this, and it always seemed like a bad decision to me. The performance benefits of most checks are trivial compared to the major downside of not testing the exact code you release. Several times I've had to track down bugs that have been very difficult to find, as they were 'fixed' by side effects of assert statements and hence didn't show up in debug builds. If a check really can't be performed in the release version for performance reasons, just separate it out into a unit test.

minipci1321 10y ago
First part of your message seem to be in contradiction with the second one, but anyways -- keeping asserts in the release image and incurring the penalty of added checks (execution speed + all these text strings stored in the read-only segment) does not make sense without the release-approving validation providing the exact coverage of all tested code. That can be tough to obtain.
jasode 10y ago
> bugs that have been very difficult to find, as they were 'fixed' by side effects of assert statements and hence didn't show up in debug builds. In those cases, the assert() was used incorrectly. However, that's not a reason to include the assert code in release builds. In a similar way, source code will often have troubleshooting code such as: #if _DEBUG // or #ifndef NDEBUG std::cerr << "inspect size: " << invoices.size() << std::endl; // more debugging-related code ... #endif Just because some programmers mistakenly put unintended side-effects in between the #if_DEBUG/#endif doesn't mean we want that code in the release builds. The purpose of assert() and _DEBUG is to not include them in release builds.
- marcosdumay 10y ago
  You can't change your program without inserting some kind of side effect. It may be timing in a multi thread system, extra memory available for an out of bounds array read, or even the CPU being kept busy and heating up or avoiding yielding to some other program.

wruza 10y ago

The rule is simple: you never assert() things that can happen, you assert ones that can not.