Just a little wtf



  • Some C code found while looking for a bug in a production library:

    int doFoo(myType_t *p)
    {
        int rtn = SUCCESS;
        if (!p)
            return rtn = FAILURE;
    ...
    

    We're leaving anyway, why set the local on the way out?

    No biggie, won't cause cancer or a core meltdown, but it did in fact elicit an audible WTF?!?!?



  • its C code, ANYTHING could happen!



  • @Algorythmics said:

    its C code, ANYTHING could happen!

     

    You know why they call it 'C' right? Cause after you read some of it, you 'C' your way to finding a new line of work.


  • Considered Harmful

    @rstinejr said:

    why set the local on the way out at all?

    It seems like the whole purpose of the rtn variable is to avoid multiple exit points (such as the one shown). My guess is it was originally just rtn = FAILURE and someone "optimized" it with an early return.



  •  @joe.edwards said:

    @rstinejr said:
    why set the local on the way out at all?

    It seems like the whole purpose of the rtn variable is to avoid multiple exit points (such as the one shown). My guess is it was originally just rtn = FAILURE and someone "optimized" it with an early return.

     Man, I remember doing a bigger C#-Project with a team of about 10 people. We all came from C-backgrounds, but we had one guy on the team who was a bit too much in love with the coding style guide from C 101. So no matter how deep we got into OOP-patterns, he kept objecting and "fixing" all multiple returns that people used. The debates got so toxic he had to be relegated to documentation.

     He kept sending mass e-mails about changing

    if (bla == "sth")  return true

    else return false

     to using temp variables and then returning that.

     



  • Here's a verbatim snippet from a different module in the same library. It's annoying enough to be distracting, but then I'm a little ADHD:

        // return result
        return result;
    }
    


  • @fire2k said:

    if (bla == "sth")  return true

    else return false

    boolean retval = bla == "sth";
    if (retval == true)
        return true;
    else if (retval == false)
        return false;
    else
        return retval;


  • @Ben L. said:

    @fire2k said:

    if (bla == "sth")  return true

    else return false

    boolean retval = bla == "sth";
    if (retval == true)
        return true;
    else if (retval == false)
        return false;
    else
        return retval;

    Having to maintain a code base (or rather, several) that make free use of this technique, it immediately raises the bile from the back of my throat.

    [code] return (bla == "sth"); //STOP BEING STUPID[/code]



  • @mikeTheLiar said:

    <font face="Lucida Console" size="2"> return (bla == "sth"); //STOP BEING STUPID</font>

     

     

    I am aware that this exists, but sometimes the readability increase from changing to it is hardly worth it considering the productivity loss from:

    - Group-Wide angry e-mails demanding this to be changed ASAP!!!111eleven

    - Several flame wars about how multiple returns and breaking out of loops is the devil

    - The several cock-ups when 8 people work on exactly that file and have to put up with merging pointless crap over TFS

    - Spamming the TFS-Logs with several "code standard fixes"

     


  • Java Dev

     I am a C developer, and I say there's nothing wrong with multiple return: there's nothing wrong with returning at the top or at the bottom a couple of times. If you need a return in the middle of a function, or a break in the middle of a loop, though, you need to split up your functions because they're getting too hard to follow. I do carry over this sentiment into other languages.



  • @mikeTheLiar said:

    return (bla == "sth"); //STOP BEING STUPID

    I seem to remember a language that would strip out the brackets in this line, but making it return bla then trying to compare to the string, making it an error. Temporary variables and/or constant returns was the only way to go in this case. Language was ActionScript 2.



  • @fire2k said:

    @mikeTheLiar said:

    <font face="Lucida Console" size="2"> return (bla == "sth"); //STOP BEING STUPID</font>

     

     

    I am aware that this exists, but sometimes the readability increase from changing to it is hardly worth it considering the productivity loss from:

    - Group-Wide angry e-mails demanding this to be changed ASAP!!!111eleven

    - Several flame wars about how multiple returns and breaking out of loops is the devil

    - The several cock-ups when 8 people work on exactly that file and have to put up with merging pointless crap over TFS

    - Spamming the TFS-Logs with several "code standard fixes"

     


    I will admit that I have a bit of a hair-trigger when it comes to stuff like this. See a previous thread of mine for a particularly egregious example.



  • bool MightBeNull(object o) {
        bool retval = false;
        return true;
    }
    


  • @rstinejr said:

    Some C code found while looking for a bug in a production library:

    int doFoo(myType_t *p)
    {
        int rtn = SUCCESS;
        if (!p)
            return rtn = FAILURE;
    ...
    

    We're leaving anyway, why set the local on the way out?

    No biggie, won't cause cancer or a core meltdown, but it did in fact elicit an audible WTF?!?!?

    I don't think this is doing what someone thinks it is. the return value is not the value of failure, it is the int value of true, because according to the line return rtn = FAILURE it attempts to assign FAILURE to the variable rtn then returns the value of wether or not that succeded.
    So essentially you get either SUCCESS or true, you never get FAILURE.


  • Java Dev

    That depends all on your language. In C, the result of the = operator is the value that is assigned, so the statement 'a=b=c=16' sets a, b, and c to 16.



  • @mikeTheLiar said:

    return (bla == "sth"); //STOP BEING STUPID

    return bla == "sth"; //RETURN IS NEITHER A FUNCTION NOR AN OPERATOR



  • @PleegWat said:

    That depends all on your language. In C, the result of the = operator is the value that is assigned, so the statement 'a=b=c=16' sets a, b, and c to 16.
     

    Double WTF points if these variables are different types.

    Treble WTF points if these variables are declared volatile. 



  • @flabdablet said:

    @mikeTheLiar said:
    return (bla == "sth"); //STOP BEING STUPID

    return bla == "sth"; //RETURN IS NEITHER A FUNCTION NOR AN OPERATOR

    return (bla == "sth"); //PARENTHESES ARE USED FOR GROUPING


  • ♿ (Parody)

    @studog said:

    @flabdablet said:
    @mikeTheLiar said:
    return (bla == "sth"); //STOP BEING STUPID

    return bla == "sth"; //RETURN IS NEITHER A FUNCTION NOR AN OPERATOR

    return (bla == "sth"); //PARENTHESES ARE USED FOR GROUPING

    #define STH "sth"
    ...
    return (bla == STH); // DON'T USE MAGIC STRINGS
    


  • @boomzilla said:

    @studog said:
    @flabdablet said:
    @mikeTheLiar said:
    return (bla == "sth"); //STOP BEING STUPID
    return bla == "sth"; //RETURN IS NEITHER A FUNCTION NOR AN OPERATOR
    return (bla == "sth"); //PARENTHESES ARE USED FOR GROUPING

    #define STH "sth"
    ...
    return (bla == STH); // DON'T USE MAGIC STRINGS
    

     

    Needs to be more Enterprisy.  I don't see a factory class loading XML through AJAX to set the value of STH here.


     


  • ♿ (Parody)

    @DCRoss said:

    @boomzilla said:

    @studog said:
    @flabdablet said:
    @mikeTheLiar said:
    return (bla == "sth"); //STOP BEING STUPID

    return bla == "sth"; //RETURN IS NEITHER A FUNCTION NOR AN OPERATOR

    return (bla == "sth"); //PARENTHESES ARE USED FOR GROUPING

    #define STH "sth"
    ...
    return (bla == STH); // DON'T USE MAGIC STRINGS
    

    Needs to be more Enterprisy.  I don't see a factory class loading XML through AJAX to set the value of STH here.

    I wanted to leave something for the next guy.



  • @fire2k said:

    if (bla == "sth")  return true
     

    Nothing wrong with

    return (blah=="sth");
    from a code style standpoint in my book.

     

    The review comment I would give for this is, "Are you sure you want to compare something to the address of a static string?"



  • @too_many_usernames said:

    The review comment I would give for this is, "Are you sure you want to compare something to the address of a static string?"

    Has anyone actually mentioned which language that snippet is? There are lots of languages where ("string" == "stri" + "ng") is true.



  • @too_many_usernames said:

    @fire2k said:

    if (bla == "sth")  return true
     

    Nothing wrong with

    return (blah=="sth");
    from a code style standpoint in my book.

     

    The review comment I would give for this is, "Are you sure you want to compare something to the address of a static string?"

    Only in Java. .NET overrides the == operator for strings.

    As an aside, starting a sentence with ".NET" just looks weird. Those periods should not be.



  • @Ben L. said:

    Has anyone actually mentioned which language that snippet is?

    @rstinejr in the op said:

    Some C code found while looking for a bug in a production library:



  • @locallunatic said:

    @Ben L. said:

    Has anyone actually mentioned which language that snippet is?

    @rstinejr in the op said:

    Some C code found while looking for a bug in a production library:

    @fire2k said:

     @joe.edwards said:

    @rstinejr said:
    why set the local on the way out at all?

    It seems like the whole purpose of the rtn variable is to avoid multiple exit points (such as the one shown). My guess is it was originally just rtn = FAILURE and someone "optimized" it with an early return.

     Man, I remember doing a bigger C#-Project with a team of about 10 people. We all came from C-backgrounds, but we had one guy on the team who was a bit too much in love with the coding style guide from C 101. So no matter how deep we got into OOP-patterns, he kept objecting and "fixing" all multiple returns that people used. The debates got so toxic he had to be relegated to documentation.

     He kept sending mass e-mails about changing

    if (bla == "sth")  return true

    else return false

     to using temp variables and then returning that.

     



  • @Ben L. said:

    @locallunatic said:

    @Ben L. said:

    Has anyone actually mentioned which language that snippet is?

    @rstinejr in the op said:

    Some C code found while looking for a bug in a production library:

    @fire2k said:

     @joe.edwards said:

    @rstinejr said:
    why set the local on the way out at all?

    It seems like the whole purpose of the rtn variable is to avoid multiple exit points (such as the one shown). My guess is it was originally just rtn = FAILURE and someone "optimized" it with an early return.

     Man, I remember doing a bigger C#-Project with a team of about 10 people. We all came from C-backgrounds, but we had one guy on the team who was a bit too much in love with the coding style guide from C 101. So no matter how deep we got into OOP-patterns, he kept objecting and "fixing" all multiple returns that people used. The debates got so toxic he had to be relegated to documentation.

     He kept sending mass e-mails about changing

    if (bla == "sth")  return true

    else return false

     to using temp variables and then returning that.

    Oh, my bad.  Sorry about that, I was thinking that it grew out of the OP due to not following along closely.



  • @fire2k said:

    Man, I remember doing a bigger C#...

    Bah I guess I missed that.

    Stupid operator overloading...

    edit: got ninja'd by locallunatic!



  • @too_many_usernames said:

    edit: got ninja'd by locallunatic!

    Are you accusing locallunatic of being Hanzo? Careful, that could be considered defamation of character.


  • ♿ (Parody)

    @HardwareGeek said:

    Careful, that could be considered defamation of character.

    Are we not doing that around here any more?



  • @HardwareGeek said:

    @too_many_usernames said:

    edit: got ninja'd by locallunatic!

    Are you accusing locallunatic of being Hanzo? Careful, that could be considered defamation of character.
     

    Uh, so comparing me to someone who is crazy is defamation?  I'd recommend taking another look at my username and/or relooking up the definition of defamation.



  • @locallunatic said:

    @HardwareGeek said:

    @too_many_usernames said:

    edit: got ninja'd by locallunatic!

    Are you accusing locallunatic of being Hanzo? Careful, that could be considered defamation of character.

     

    Uh, so comparing me to someone who is crazy is defamation?  I'd recommend taking another look at my username and/or relooking up the definition of defamation.

    For anyone too lazy to Google define:lunatic,


  • Discourse touched me in a no-no place

    @Helix said:

    @PleegWat said:

    That depends all on your language. In C, the result of the = operator is the value that is assigned, so the statement 'a=b=c=16' sets a, b, and c to 16.
     

    Double WTF points if these variables are different types.

    Treble WTF points if these variables are declared volatile. 

    No points for misunderstanding what volatile does.


  • Discourse touched me in a no-no place

    @too_many_usernames said:

    @fire2k said:

    if (bla == "sth")  return true
     

    Nothing wrong with

    return (blah=="sth");
    from a code style standpoint in my book.

     

    The review comment I would give for this is, "Are you sure you want to compare something to the address of a static string?"

    In C, which it's been pointed out this snippet is, you might conceivably want to do that. Perhaps you're running on an embedded system and memory is tight.

    It's probably not a good idea to do in general but it could be legit.


  • Discourse touched me in a no-no place

    @FrostCat said:

    In C, which it's been pointed out this snippet is, you might conceivably want to do that. Perhaps you're running on an embedded system and memory is tight.
    If memory is tight enough to make this even vaguely a good idea, it's better to use magic integer values instead (as you can pick a smaller datatype than the local pointer if necessary). If you've got the space to waste on strings (more than 64kB!) you've got the space to not need address comparisons with a literal.



  • @FrostCat said:

    No points for misunderstanding what volatile does.
     

    As far as I know, it does absolutely nothing on any modern compiler.


  • Considered Harmful

    @Mcoder said:

    @FrostCat said:

    No points for misunderstanding what volatile does.
     

    As far as I know, it does absolutely nothing on any modern compiler.


    C and C++ were more than a decade ago for me, but I thought volatile class members could be modified even from a const method, and that the compiler would never optimize away reads or assume the value has not changed.



  • @joe.edwards said:

    the compiler would never optimize away reads or assume the value has not changed.
    This. It is necessary for data that may be modified by things like interrupt service routines, or even hardware registers, because the value you have in a CPU register may be stale. Much used in device drivers.



  • @FrostCat said:

    @too_many_usernames said:

    @fire2k said:

    if (bla == "sth")  return true
     

    Nothing wrong with

    return (blah=="sth");
    from a code style standpoint in my book.

     

    The review comment I would give for this is, "Are you sure you want to compare something to the address of a static string?"

    In C, which it's been pointed out this snippet is, you might conceivably want to do that. Perhaps you're running on an embedded system and memory is tight.

    It's probably not a good idea to do in general but it could be legit.

     

     It's C# (which a lot of people managed to read), so have fun actually comparing real string pointers. Maybe if you declared the file unsafe and started to actually use pointers.

    object.ReferenceEquals() would probably be the fastest sane way.

    Or if you really wanted to get technical you could probably start mentioning String Pools/Internal Pools and how every literal string is/should be the same handle anyway.

     Applause for ressurecting the embedded filesystem-meme, however.

     

     


Log in to reply