Null check what you shall not!



  • And the parade of terrible code continues:
    [code]
    // Widget.H
    class Widget { /.../ }; // note: this class has no comparison operators defined for it
    typedef std::list<Widget> listOfWidgets;

    // SomeClass.C
    void SomeClass::SomeDumbFunction(void)
    {
    listOfWidgets::iterator i = someWidgets.begin();
    while (someWidgets.end() != i)
    {
    if (i != 0)
    {
    putzWithAWidget(i);
    i++;
    }
    }
    /
    ... */
    }
    [/code]

    So...WTFs:

    • Indentation -- this, and many other indentation WTFs such as randomly indented lines of code, are endemic in the codebase, but that's not even close to TRWTF here
    • The use of a while loop when a for loop would have made for more readable code -- why they do it this way, I have no clue...but they do, in many places, too
    • The postincrement on iterators -- just plain silly in a void context, as there's no disadvantage to preincrement
    • Finally, and how I found this snippet: the if block comparing the iterator against 0 before it's dereferenced. Standard library iterators are not pointers and do NOT need to be checked against NULL! Modern versions of G++ will not find the operator!= that the if block is calling for...


  • @tarunik said:

    Indentation -- this, and many other indentation WTFs such as randomly indented lines of code, are endemic in the codebase, but that's not even close to TRWTF here

    Ctrl + K, Ctrl + D

    Any good IDE should have that.



  • Indeed; we plan to auto-format the code after we get the 'make it compile' commit put in, because I suspect that format changes mixed with 'make it compile' changes would make some people mad for reasons I cannot explain.



  • How about the reformat before the 'make it compile' commit so that your make-it-compile is less painful?



  • @Arantor said:

    How about the reformat before the 'make it compile' commit so that your make-it-compile is less painful?

    The formatting is bad, just not eye-bleedingly bad enough to merit such a drastic step; it's also so endemic in all of their code that we can't push anything back to $vendor that has such drastic reformatting in it without getting screamed at for weeks on end.



  • It was just a thought. But I can understand this.



  • Additional wtf; it should be const_iterator



  • We use to have a guy that used to use this pattern

    try
    {
        //Snip code
    } 
    catch (NullReferenceException nex) 
    {
        //some logging code
    }
    

    This was frequently used in on the RepeaterItemDataBound Event handler so everytime a html list was generated it would log a set of exceptions. It was infuriating.


  • Discourse touched me in a no-no place

    @tarunik said:

    Indeed; we plan to auto-format the code after we get the 'make it compile' commit put in, because I suspect that format changes mixed with 'make it compile' changes would make some people mad for reasons I cannot explain.

    It makes the diffs difficult to read. Determining the set of changes that 'make it compile' from the set of changes that result from 'format' is easier if they're in two different commits.



  • Wow, at least he didn't use the generic Exception like most people would.



  • He changed it depending on whether another exception could occur. Another pair of developers (they are still churning out code) use my favourite pattern in an MVC controller:

    try {
       //some code
    } 
    catch (Exception e)
    {
        throw new Exception(Json.Serialize(e));
    }
    

    Being an MVC action it you had JSON inside a JSON object.


  • kills Dumbledore

    I've just found this:

            catch (Exception e)
            {
                // try-catch so that the open connections are closed
                throw new Exception(e.Message);
            }
    

    I think it's a weird cludge to make sure the finally block below it runs, but this is a pretty major code smell to me



  • If you're using a Close() statement in C#, you're Doing It Wrong™.


  • kills Dumbledore

    finally
    {
    xmlreader.Close(); xslreader.Close();
    xmlstream.Close(); xslstream.Close();
    output.Close();
    }
    This should all be replaced with a using statement shouldn't it?



  • Yuuuuuuuuuuuup.



  • @tarunik said:

    * Indentation -- this, and many other indentation WTFs such as randomly indented lines of code, are endemic in the codebase, but that's not even close to TRWTF here

    The indentation looks as if it's done with tab width 8, and you have tab width 4.

    The postincrement on iterators -- just plain silly in a void context, as there's no disadvantage to preincrement
    They are precisely as silly as preincrement. There's no reason one is better than the other.

  • Banned

    @Hanzo said:

    There's no reason one is better than the other.

    Actually, there is a very good reason why pre-increment is better in C++ - it's because post-increment forces creation of a copy of the iterator.

    Non-void assignment and increment expressions were probably the biggest mistake in C language design, and C++ should cut this stupid shit - if not in C++98, then at least in C++03.

    Oh, and C++11 should make not-really-copyable iterators actually non-copyable, now that we have move semantics.



  • @Gaska said:

    Non-void assignment and increment expressions were probably the biggest mistake in C language design

    Except in C, the compiler would easily optimize the copy out for any of the types. In C++, however, it needs to preserve it in case your ++ operator does a Riverdance instead of an increment.


  • Banned

    I wasn't talking about performance issues as much as potential for abominations like

    while (i = thingsLeft()) doSomething(i);


  • @Gaska said:

    while (i = thingsLeft())

    It's a pretty cool idiom. At least until that one day when you misread = as ==.


  • Banned

    @Maciejasjmj said:

    It's a pretty cool idiom

    I hate you.


  • Java Dev

    That'll give you a compiler warning in GCC (at least for C code, probably C++ as well). Assignment used as truth value should be reinforced with an extra set of brackets.

    if ((r = foo_twiddle()))
    {
        log_error("Could not twiddle foo: %d\n", r);
        return -1;
    }
    


  • @Maciejasjmj said:

    It's a pretty cool idiom. At least until that one day when you misread = as ==.

    while ($row = mysql_fetch_assoc($query))
    {
        // do stuff
    }
    

  • Banned

    @PleegWat said:

    Assignment used as truth value should be explicitly compared to zero.

    FTFY. Seriosly, get your types straight. Just because something is possible doesn't mean you should do it.


  • Java Dev

    Ugh no. That's just visual bloat. Next you'll be defending our UI lead who is a big defender of this antipattern:

    if ( true === $objFoo->IsTwiddled() )
    {
        ....
    }
    

    Where naturally IsTwiddled will only return true or false


  • Banned

    @PleegWat said:

    Where naturally IsTwiddled will only return true or false

    Here. This makes whole world of difference. if statement is designed to work on boolean expressions. Calling a function that returns boolean value is boolean expression. Calling a function that returns a numerical value is not boolean expression. Explicitly comparing boolean expression to boolean constant is absolutely redundant and stupid. Explicitly comparing numerical expression to numerical constant is how you fucking make a boolean expression that should be used in if statement which works on boolean expressions FFS!!!!!

    And don't tell me that in C there's no boolean type and if statement is actually working on numerical expressions - for this is just an implementation detail that has been exposed to the interface, breaking all laws of encapsulation, type safety, logic and sanity. Same with having single type for one-byte number and character.


  • Java Dev

    Well, obviously I don't need to tell you because you already know...

    I think question is when is something boolean context. Error code return values (which are 0 for success) I treat as boolean. Same with pointers. If I'm checking a count for zero I include the ==0. I usually write !strcmp() for comparisons though that can be kind of a pitfall as well.
    You can call it inconsistent but I like to think I'm consistent about it...



  • @Gaska said:

    Actually, there is a very good reason why pre-increment is better in C++ - it's because post-increment forces creation of a copy of the iterator.

    Non-void assignment and increment expressions were probably the biggest mistake in C language design, and C++ should cut this stupid shit - if not in C++98, then at least in C++03.

    Oh, and C++11 should make not-really-copyable iterators actually non-copyable, now that we have move semantics.


    OK, the pre-increment has been implemented in a morose way, but that's just the library you're using. If it only uses operator overloading to achieve a vague resemblance to C pointer semantics, and gives you a performance hit, the designer should be taken out and shot. Instead, ++() could be implemented like this:

    T* t = iter.currentValue();
    iter.next();
    return t;
    

    If ++ and currentValue are inlined, they can be optimized away.

    Non-void assignment and increment expressions were probably the biggest mistake in C language design, and C++ should cut this stupid shit
    I'd say C++ was the biggest mistake in C language design.

  • Banned

    @Hanzo said:

    Instead, ++() could be implemented like this:

    Good luck doing this with non-container iterator (e.g. generator). Also, even in this optimal way you have to create a new value. Also, your approach becomes WTF as soon as someone uses the result of operator++(int).

    @PleegWat said:

    You can call it inconsistent but I like to think I'm consistent about it...

    Yes, you're pretty consistent. I would still use ==0, though - but it certainly doesn't bring in much.

    @Hanzo said:

    I'd say C++ was the biggest mistake in C language design

    C++ isn't C. And both have implicit cast - another bad design decision.



  • @Gaska said:

    Good luck doing this with non-container iterator (e.g. generator). Also, even in this optimal way you have to create a new value. Also, your approach becomes WTF as soon as someone uses the result of operator++(int).

    Doesn't have to be. Both operators can be implemented in the same fashion. If the library's iterator design can't provide that, it shouldn't have a ++ operator. C++ (ha!) doesn't look like C at all anymore, so there's no reason to hold on to the ++ notation at all costs.



  • @Gaska said:

    Here. This makes whole world of difference. if statement is designed to work on boolean expressions. Calling a function that returns boolean value is boolean expression. Calling a function that returns a numerical value is not boolean expression. Explicitly comparing boolean expression to boolean constant is absolutely redundant and stupid. Explicitly comparing numerical expression to numerical constant is how you fucking make a boolean expression that should be used in if statement which works on boolean expressions FFS!!!!!

    I recently changed implicit conversion from None to Error in a VB .NET project, and it exposed the latter anti-pattern. Man, my cluebat got a lot of use that day...



  • In C there's no boolean type; while and if statements actually test numerical expressions.

    This is because C was originally designed as a portable alternative for the kinds of logic formerly implemented in various assembly languages. As originally conceived it was a language completely devoid of trainer wheels, designed to let careful and competent coders express correct logic concisely. Safety features for the careless were not in its original scope.

    There's a terse, stark beauty in Unix v6 era C that went completely out the window once ANSI got their hands on the language. C++ took that uglification process and ran with it to somewhere very close to its logical extreme.

    To my way of thinking, if you're coding in C, you're doing that for a reason and you ought to be expressing your ideas in C, not in some bastardized C-like pseudocode. Treating a C conditional as if it were in fact boolean rather than arithmetic stems from the same kind of feeble-mindedness as the desire to write

    #define begin {
    #define end }

    and I have no respect for it whatsoever. There is nothing at all wrong with

    while (*d++ = *s++);

    and anybody who says different is not a proper programmer.


  • Banned

    @Gaska said:

    And don't tell me that in C there's no boolean type and if statement is actually working on numerical expressions

    @flabdablet said:
    In C there's no boolean type; while and if statements actually test numerical expressions.

    -____-


  • Java Dev

    This. When I'm writing C code (which is a lot of the time, and of the languages we use it's the one I enjoy most), I'm trying to do what I want as clearly as possible while using as little code as possible.

    I spent quite some idle time this week thinking up a better solution to define both a large enumeration and function mapping its ids to stringified versions of its labels, without writing the whole thing out twice. I ended up going with my initial idea - a shell script generating a .h and a .c file. I'm still not sure if I like the result.


  • Banned

    @flabdablet said:

    This is because C was originally designed as a portable alternative for the kinds of logic formerly implemented in various assembly languages.

    And this is the source of all the bad decisions made in design of C - it's far too close to the metal for a general purpose language. This, and brevity. Brevity is good when it makes code easier to read. Assignments and increments in the middle of expressions aren't easy to read.

    @flabdablet said:

    To my way of thinking, if you're coding in C, you're doing that for a reason

    Usually interoperability with existing libraries. Because in 2014 C is horrible idea for anything but compatibility with pre-existing stuff and kernel programming.

    @flabdablet said:

    Safety features for the careless were not in its original scope.

    Excuse me but isn't C statically typed?

    @flabdablet said:

    There's a terse, stark beauty in Unix v6 era C that went completely out the window once ANSI got their hands on the language.

    Iunno, I'm not old enough to remember that. But I doubt there weren't implicit casts and defaulting to int in it.

    @flabdablet said:

    C++ took that uglification process and ran with it to somewhere very close to its logical extreme.

    And added facilities for easy OOP. Also, RAII.

    @flabdablet said:

    and you ought to be expressing your ideas in C

    if (i != 0) is valid C.

    @flabdablet said:

    Treating a C conditional as if it were in fact boolean rather than arithmetic stems from the same kind of feeble-mindedness as the desire to write [censored Pascal]

    As I said before, conditional statements working on numerics is because of how assemblers work - ie. implementation detail that shouldn't be exposed to the interface but is.

    @flabdablet said:

    There is nothing at all wrong with

    while (*d++ = *s++);


    Yes there is. Operator precedence is unobvious. Yes, it's well defined and you get used to it quickly, but it still is unobvious. But I'm the kind of guy who puts parens in (a && b) || c.

    @flabdablet said:

    and anybody who says different is not a proper programmer.

    Oh, so real programmers never say that C is flawed? Should I also switch to vim to not lose my programmer license?


  • Discourse touched me in a no-no place

    @PleegWat said:

    When I'm writing C code (which is a lot of the time, and of the languages we use it's the one I enjoy most), I'm trying to do what I want as clearly as possible while using as little code as possible.

    +1 for “clearly as possible”.
    -1 for “as little code as possible”.

    Sacrificing clarity for brevity can all too easily lead to code that is in error, especially when wielded by a some poor maintenance programmer in a few years time. Make it clear, then as a secondary objective make it possible for the compiler to make it fast for you. (Assuming you picked the right algorithm…)


  • BINNED

    @Gaska said:

    Excuse me but isn't C statically typed?

    It's statically typed, but not strongly typed. It will do implicit type conversions all over the place and whatever explicit casts you want. Whether or not the types can be meaningfully converted is your problem. You need both static typing and strong typing for safety.



  • This is, incidentally, one of PHP's very nasty hidden gotchas.



  • @PleegWat said:

    I spent quite some idle time this week thinking up a better solution to define both a large enumeration and function mapping its ids to stringified versions of its labels, without writing the whole thing out twice. I ended up going with my initial idea - a shell script generating a .h and a .c file. I'm still not sure if I like the result.

    Abuse the preprocessor.

    /* gen-enum-values.h */
    #undef ENUM
    #define ENUM(NAME) enum NAME {
    #undef V
    #define V(VALUE) VALUE,
    #undef END
    #define END }
    
    /* gen-enum-tostring-c.h */
    #undef ENUM
    #define ENUM(NAME) \
    const char *#NAME ## _tostring(enum NAME v) { \
        static const char *lookup[] = {
    #undef V
    #define V(VALUE) #VALUE,
    #undef END
    #define END \
        }; \
        return lookup[v]; \
    }
    
    /* gen-enum-tostring-h.h */
    #undef ENUM
    #define ENUM(NAME) \
    const char *#NAME ## _tostring(enum NAME v);
    #undef V
    #define V(VALUE)
    #undef END
    #define END
    
    /* enum-definitions.h */
    ENUM(foo)
    V(pleeg)
    V(wat)
    END
    
    ENUM(bar)
    V(flab)
    V(dab)
    V(let)
    END
    
    /* use-enums.h */
    #include "gen-enum-values.h"
    #include "enum-definitions.h"
    #include "gen-enum-tostring-h.h"
    #include "enum-definitions.h"
    
    /* use-enums.c */
    #include "use-enums.h"
    #include "gen-enum-tostring-c.h"
    #include "enum-definitions.h"
    

    ...or something along those lines. It's been a while.



  • @Gaska said:

    And this is the source of all the bad decisions made in design of C - it's far too close to the metal for a general purpose language.

    My point is that a lot of C's design decisions only look bad if you think of C as a general purpose application language, rather than an assembler-replacing systems programming language.

    A drill bit makes an incredibly badly designed screwdriver.



  • @Gaska said:

    As I said before, conditional statements working on numerics is because of how assemblers work

    No, it's because of how processors work. The point of a systems programming language is to make that level of abstraction available.

    Consider fucking off, @codinghorror.


  • Discourse touched me in a no-no place

    @flabdablet said:

    Consider fucking off, @codinghorror.

    @Codinghorror probably never handled Usenet well.


  • Banned

    Neither did normal human beings.



  • What would you know about them?



  • Plus it loses the inner exception's information. Very bad.


  • Discourse touched me in a no-no place

    @codinghorror said:

    Neither did normal human beings.

    What have normal human beings got to do with TDWTF?


  • Discourse touched me in a no-no place

    Your instincts are pretty accurate: that code stinks in several ways at once. C# has finally. Closing things manually is for morons who don't use resources. Shredding the information in the exception so that nobody can find out what happened is awful. Throwing a generic exception instead of something more specific just puts a special frosting on the whole 💩 sandwich.


  • kills Dumbledore

    Looking through the commit logs, the close statements were originally at the end of the try block, moving them to the finally was one of the first things I did when I started on the project. The reason for the explicit close rather than using blocks is because the readers and writers are initialised differently depending on various conditions. If I get time I'll probably refactor it so the initialisation is in a separate method and everything will work fine with using



  • @blakeyrat said:

    What would you know about them?

    Everything @codinghorror knows is based on experiments using fuckus groups.


  • ♿ (Parody)

    @flabdablet said:

    Everything @codinghorror knows is based on experiments using fuckus groups.

    I think a top quoted signature block once killed his dog.


Log in to reply