• (nodebb)

    The obvious WTF is a function that returns true/false for fuccess or sailure. (I put it like that because there's no way to tell which way round a particular function is, since people use both true-success and false-success in the same codebase. In general, though, people who do that should be shot.)

    There's a case for false=success, because that way you can easily change to a numeric code and keep "!f(params)" meaning "f succeeded.

    There's also a case for using an enum all of whose members are NOT zero, including success, precisely to discourage that.

    Or you use an object that asserts if it is not checked. And then people will write code like this: f(params).succeeded();. This is the voice of experience speaking. We had a framework that included such a status class, and one guy on the team would write that just to stop the code from exploding.

    There's NO easy solution to this. The solution that is the least likely to be disastrous is to pass the return status as an "out" parameter rather than returning it, although that tends to annoy people who call the interface because they can't be lazy about things.

  • Quite (unregistered)

    My current workload concerns a backend which returns a status report where the convention is precisely the opposite: 1 for success, and 0 for failure -- along with a complex data structure containing all there is to know of any importance about the nature of the failure.

    Fortunately it is rigorously consistent, so just as soon as I found someone who could explain what all that data structure meant, and how it should be treated at the front end for each category of error, coding up the error page became a straightforward task that could be considered done and dusted.

    The moral of the story, ladies and gentlemen, is that the guy who originally stated something like "A foolish consistency is the hobgoblin of little minds" was no software engineer. Quick google: Emerson. That explains it.

    "Good, he did not have enough imagination to become a mathematician". -- Hilbert

  • Paul (unregistered)

    I think the original coder was trying to check if all of those returned true, and return that result. Of course using &= would have been a better solution...

  • (nodebb) in reply to Quite

    To be fair to Mr. Emerson, he did specify a "foolish" consistency. Much of the time, consistency is anything but foolish.

  • EvilSnack (unregistered)

    I beg to differ. The purpose of code comments is to minimize the cost of code maintenance, and it does this by declaring what the code is supposed to be doing. That way, when the code has to be changed by someone who is seeing it for the first time, they can find the code block that is supposed to be doing X, either so that they can code it to do Y, or fix it so that it really is doing X.

  • (nodebb)

    It's only redundant placing the same comment after each statement. There should be only one instance of the comment - perhaps after NotResult is defined. The rest of the code is rather clear, and the existence of the comment after each statement is a real PITA when that stuff word-wraps.

    I do not agree with Paul's statement regarding &= rather than |=. If the code utilized &= there would be a failure only if all of the updates fail. Using |= will flag a failure if any of the updates fail, which is more likely to be a case to flag a failure.

    TRWTF: If only one of those template updates fails, we have !Success. Great, we know it didn't work. However, we don't know which update failed, and so we have no way to roll anything back. True, we can't see what happens when this returns, but I doubt there is any type of cleanup if the coder has no way to signal where the actual failure occurred.

  • Jeff Dege (unregistered)

    I think my favorite rule-of-thumb in this area is:

    "If the code doesn't match the comments, they're both probably wrong"

  • Necropaw (unregistered) in reply to Bananafish

    @Bananafish: Paul is right. Using the De Morgan's laws the whole code can be simplified. Instead of "return !(!A | !B | !C) " it can be transformed into "return A & B & C"

  • Brad Wood (google)

    As Homer would say, "I'm not not licking toads."

  • Carl Witthoft (google)

    Or the other old saying: "Two wrongs don't make a right, but three lefts do."

  • Dave (unregistered)

    As others have said, surely something like this could be done instead? :

    static BOOLEAN UpdateFileStoreTemplates () { BOOLEAN Result = TRUE;

    /* attempt all updates and return FALSE if any were unsuccessful */ Result &= UpdateFileStoreTemplate (DC_EMAIL_TEMPLATE); Result &= UpdateFileStoreTemplate (DC_TABLE_HEADER_TEMPLATE); Result &= UpdateFileStoreTemplate (DC_TABLE_ROW_TEMPLATE); Result &= UpdateFileStoreTemplate (DC_TABLE_FOOTER_TEMPLATE); Result &= UpdateFileStoreTemplate (WS_EMAIL_TEMPLATE); Result &= UpdateFileStoreTemplate (WS_TABLE_HEADER_TEMPLATE); Result &= UpdateFileStoreTemplate (WS_TABLE_ROW_TEMPLATE); Result &= UpdateFileStoreTemplate (WS_TABLE_FOOTER_TEMPLATE);

    return Result; }

  • Dave (unregistered)

    Sorry, didnt know the formatting would do that... in future, should it be surrounded by bla or somethine like that?

  • Dave (unregistered)

    As others have said, surely something like this could be done instead? : static BOOLEAN UpdateFileStoreTemplates () { BOOLEAN Result = TRUE;

    /* attempt all updates and return FALSE if any were unsuccessful */ Result &= UpdateFileStoreTemplate (DC_EMAIL_TEMPLATE); Result &= UpdateFileStoreTemplate (DC_TABLE_HEADER_TEMPLATE); Result &= UpdateFileStoreTemplate (DC_TABLE_ROW_TEMPLATE); Result &= UpdateFileStoreTemplate (DC_TABLE_FOOTER_TEMPLATE); Result &= UpdateFileStoreTemplate (WS_EMAIL_TEMPLATE); Result &= UpdateFileStoreTemplate (WS_TABLE_HEADER_TEMPLATE); Result &= UpdateFileStoreTemplate (WS_TABLE_ROW_TEMPLATE); Result &= UpdateFileStoreTemplate (WS_TABLE_FOOTER_TEMPLATE);

    return Result; }

  • Dave (unregistered)

    (╯°□°)╯︵ ┻━┻

  • Patrick (unregistered)

    Not if they are returning a status code or such. Remember anything that's not equal to 0 is considered true. So one function could be returning 1, another 2, and yet another 4, in which case ANDing them together would break. The BOOLEAN type here is probably just a typedef'd integer.

    It would be identical if you did !!UpdateFileStoreTemplate instead however to normalize the values.

  • CrazyEyes (unregistered) in reply to Quite

    To be pedantic, Emerson was a philosopher. He was merely saying that if you hold to an idea that you previously had simply because you'd be "inconsistent" -- even if the idea is clearly wrong -- you're a fool, not that consistency itself is a crappy virtue. A more analogous example in software engineering would be the classic clueless small company that still uses the same code base 15 years later even though it's accumulated 8000+ bugs (see recent article posted this week).

    On the subject of error codes, I personally don't really understand why folks decided that the 0 = success and other numbers = other error codes to be outdated. You can always still throw exceptions, but not every environment and/or integrated application ever allows you to properly utilize those exceptions. I don't see how it's particularly "harmful" to use an int error code, other than that the memory allocated to an int is more than that of a bool, but those memory concerns are largely irrelevant today. Especially if you've written your code well, and it doesn't make a call to read some file or touch an external piece (usually the type of function that returns an error code) inside of a massive loop.

  • Robert Hanson (unregistered)

    This is begging for a rewrite that reads better. Note that in the original code, all the save attempts are made; and if any one of them fails, the function returns fail; but it's not clear what state the underlying (presumably back end) is left in.

    succeeded = [DC_EMAIL_TEMPLATE,DC_TABLE_HEADER_TEMPLATE,DC_TABLE_ROW_TEMPLATE, ...].reduce(success,templ) { var result = UpdateFileStoreTemplate (templ); return success & result; }, true); return succeeded;

  • Shn (unregistered) in reply to Bananafish

    Bananafish, you are are incorrect and Paul is correct. Just change the initial value to TRUE, remove the ! on all lines and use &= and it would be exactly the same logic without the redundant logical NOTs.

  • I dunno LOL ¯\(°_o)/¯ (unregistered)

    All those crap comments could be gotten rid of with one simple change:
    s/NotResult/fail/g
    Then at least the variable will have a sensible name that relates to what it means in its negative logic context. In any case, using "result" as the name of a return value is dumb because it tells you nothing about what the result means.

    Even better, excessive negative logic makes code very hard to read, so De Morgan the whole thing. Change it to "bool ok = true;", change the ands to ors, and then you can untie all those silly Gordian nots and "return ok;" Now the code even makes sense when you read it. De Morgan's laws aren't that hard, I discovered it on my own at age 12 or so when playing with TTL chips. I was a little disappointed when I realized it was already a thing, but mostly I felt cool for having noticed something so basic and powerful. But maybe it was easier for me to learn in an electronics context because I could visualize the inversion bubbles morphing around on the schematic symbol, and see the effect on the truth tables.

    But is very important to realize that |= and &= are bit-wise operators, not short circuit boolean operators. I really don't like mixing booleans with bit-wise operators, because it's bad voodoo. It opens up the chance of other bits being set, causing hair-pulling behavior. IIRC there is no such thing as &&=, so each line would need to become "ok = ok && foo();" That is in fact a pattern that I know I've used before.

  • Ross (unregistered)

    The REAL WTF is, why does the syntax coloring plugin in use on this site make the letters MPLATE blue? https://www.screencast.com/t/NTQRAmbE

  • Talis (unregistered)

    New game: Try to guess the cornified word!

    I lost today, trying "bizarre" and "not-ing"...

  • chreng (unregistered)

    It feels like Not is Invented Here

  • RandomStranger (unregistered)

    This code was written by the Sheriff of NOTtingham

  • Pedant (unregistered)

    The obvious WTF is a function that returns true/false for fuccess or sailure. ... There's NO easy solution to this.

    Yeah, obviously we'd like our function to return a value that clearly indicates "ok" or "fail", if only there was a type that could perhaps enumerate terms like that. Perhaps some kind of "enumerated type"? We could call it "enum" for short and perhaps declare it as enum status_t { OK, FAIL }.

  • (nodebb) in reply to Necropaw
    @Bananafish: Paul is right. Using the De Morgan's laws the whole code can be simplified. Instead of "return !(!A | !B | !C) " it can be transformed into "return A & B & C"

    Indeed. I was stuck on the ! and was thinking Paul meant "return ! ( !A & !B & !C )" - which would be wrong.

  • (nodebb) in reply to Shn
    Bananafish, you are are incorrect and Paul is correct. Just change the initial value to TRUE, remove the ! on all lines and use &= and it would be exactly the same logic without the redundant logical NOTs.

    Yes, I understand that, but Paul's comment was to replace |= with &=, and he did not specify the many !s should be removed. I went on what he said.

  • Pedant (unregistered) in reply to Bananafish

    Yes, I understand that, but Paul's comment was to replace |= with &=, and he did not specify the many !s should be removed. I went on what he said.

    You're both neglecting the fact that the original code used !, which is logical not and thus coerces everything to 0 or 1, in combination with bitwise or. But now you want to drop the logical not and just use bitwise and. So if these functions all pass, but one of them returns a success code of 2 instead of one, the whole thing will fail.

    And you can't just use a && b && c because that short-circuits, which changes the behavior.

  • Erik (unregistered) in reply to Bananafish

    Protip: De'Morgans laws.

  • ray10k (unregistered)

    nananananananananana banana man!

  • Brian White (unregistered)

    TRWTF is that I tried to log in to post, and the result was Illegal arguments: string, object

  • Brian White (unregistered)

    The comment about logical vs bitwise operations was spot on. You can still use &=. Just throw a !! In front of each argument to coerce each to logical first :)

  • RWC (unregistered)

    @Pendant: You are wrong. It is perfectly fine to use either && or &= as long as every variable is a boolean.

    x &= y

    is an AND assignment. AND the value of y with the value of x, store the result in x, and return the new value. It has nothing to do with bitwise operations

    Some simple code (.NET C#):

    public class Program
    {
        public static void Main(string[] args)
        {
            bool a = true;
            bool b = false;
            bool c = true;
    
            bool test1 = a && b && c;
    
            Console.WriteLine(test1 ? "true" : "false");
    
            bool test2 = true;
            test2 = test2 && a;
            test2 = test2 && b;
            test2 = test2 && c;
    
            Console.WriteLine(test2 ? "true" : "false");
    
            bool test3 = true;
            test3 &= a;
            test3 &= b;
            test3 &= c;
    
            Console.WriteLine(test3 ? "true" : "false");
    
            Console.ReadKey();
        }
    }
    

Leave a comment on “Notted Up”

Log In or post as a guest

Replying to comment #:

« Return to Article