Use key to get itself



  • Since my former coworker left a couple months ago, I've been working on cleaning up some of his code. One of his projects was to maintain a handful of related services for processing message received from another company. These service are full of minor WTFs, but there are a couple that I must share. Here is the first.

    Information has been anonymized to protect, well, me.

    Before I begin, a little information on a table which is important in the WTF. There is a table in the DB called Dropdowns used to store values for dropdown lists which are used repeatedly throughout our software. It is also used to link those values to values used by the one company we have massive electronic communication with. This is it's basic structure:

     Field       | Purpose
    -------------+--------------------------------------------------------------------------------
    Dropdowns_ID | Primary key
    Dropdown_Key | Used to group values by specific dropdown.
                 | For example, "Mr.", "Mrs.", etc. belong to Dropdown_Key "NameTitle"
    Display      | The value seen by the user in a dropdown list
    Ret_Integer  | The hidden value used in dropdown lists. Also, used as the foreign key
                 | for other tables (not my choice).
    Ret_String   | The corresponding value, if any, used by the other company in communications.
    

    So yesterday I was looking over some code when I discovered this gem (methods renamed for clarity):

    int messageCode = GetReturnIntFromString("appropriateDropdownKey", messageValue);
    
    /* SNIP 50 LOC. messageCode is not modified in this time. */
    
    int returnInteger = GetReturnIntFromDropdownID(messageCode);
    

    So first, he correctly got the Ret_Int by treating the received value as a Ret_String. But then later in the code, he treated the already processed Ret_Int as if it were actually Dropdowns_ID to get a different Ret_Int. :headdesk:

    As if that weren't bad enough, I also found this "beauty" for handling a different value from the same message:

    int temp;
    Int32.TryParse(messageString, out temp);
    
    int differentReturnInteger = GetReturnIntFromDropdownID(temp);
    

    :headdesk: :headdesk: :headdesk: :headdesk:


  • Discourse touched me in a no-no place

    @abarker said:

    int temp;
    Int32.TryParse(messageString, out temp);

    int differentReturnInteger = GetReturnIntFromDropdownID(temp);

    Nice handling of error cases from that TryParse


  • FoxDev

    @abarker said:

    ```csharp
    int temp;
    Int32.TryParse(messageString, out temp);
    int differentReturnInteger = GetReturnIntFromDropdownID(temp);

    At least in C#6 it's a line shorter:
    
    ```csharp
    Int32.TryParse(messageString, out int temp);
    int differentReturnInteger = GetReturnIntFromDropdownID(temp);
    


  • You know that someone really understands the concept behind foreign keys when he's not using the primary key as a reference...



  • I wonder if Ret_Integer is usually equal to Dropdowns_ID... It looks like nothing would work if that wasn’t the case.



  • @dkf said:

    Nice handling of error cases from that TryParse…

    Not sure if ignorant or trolling ...

    Anyway, unlike Parse, TryParse handles the errors internally. When an error is encountered, TryParse returns false and the output is set to 0.



  • @VinDuv said:

    I wonder if Ret_Integer is usually equal to Dropdowns_ID.

    Rarely, if ever. The first item has Dropdown_ID of 1 and Ret_Int of 0. Since Dropdown_ID contiuously increments for the entire table, and Ret_Int resets for each list ...



  • @abarker said:

    Anyway, unlike Parse, TryParse handles the errors internally. When an error is encountered, TryParse returns false and the output is set to 0.
    And that condition is never checked for prior to GetReturnIntFromDropdownID, and abarker just confirmed my fears that there is no Dropdown_ID of 0.


  • FoxDev

    So, I think we can all agree it should be this (C# 6 again):

    if (!Int32.TryParse(messageString, out int temp))
        throw new WhatTheFuckAreYouDoingManException();
    int differentReturnInteger = GetReturnIntFromDropdownID(temp);
    


  • or you could use Int32.Parse...


  • FoxDev

    Yeah, if you don't mind having no control over the exception thrown or its contents…



  • @RaceProUK said:

    At least in C#6 it's a line shorter:

    Int32.TryParse(messageString, out int temp);
    int differentReturnInteger = GetReturnIntFromDropdownID(temp);
    

    <!-- Emoji'd by MobileEmoji 0.2.0-->

    Wait... What..? I guess we must agree that C# is TRWTF.
    That syntax is terrible. And really, inexcusable to even exist.


  • FoxDev

    @Evo said:

    That syntax is terrible. And really, inexcusable to even exist.

    I like it; useful when you need a throwaway variable. And I have used throwaway variables on many occasions.


  • 🚽 Regular

    What's so terrible about it?


  • Discourse touched me in a no-no place

    @abarker said:

    Not sure if ignorant or trolling ...

    Neither. Pointing out that when something bad happens, the failure mode will be unexpected (as a 0 is fed into the next stage of the lookup). The code should instead refuse to proceed and throw it back in the user's caller's face.



  • @dkf said:

    Neither. Pointing out that when something bad happens, the failure mode will be unexpected (as a 0 is fed into the next stage of the lookup). The code should instead refuse to proceed and throw it back in the user's caller's face.

    Ah. Gotcha.



  • @Zecc said:

    What's so terrible about it?

    The fact that it completely defeats the visual clues of how the code works. Type definitions are, in my eyes, a key part of easily understanding the code, even if they are only temporary. Personally, I never have a type definition directly underneath a line of code (except perhaps an iterator in a for-loop), and in one glance it's obvious where the variables are declared and used.
    Here, good luck finding the declaration of "temp". Okay, it's doable, but it's a lot less obvious than having "int temp;" above the two lines.



  • @RaceProUK said:

    I like it; useful when you need a throwaway variable. And I have used throwaway variables on many occasions.

    My first thought looking at that (I do c++, not c#) is that temp's scope would just be in the call, like a for loop. That it extends beyond just seems wrong.



  • @RaceProUK said:

    I like it; useful when you need a throwaway variable. And I have used throwaway variables on many occasions.

    What do your colleagues think? I think making code shorter line-wise by making the lines longer is rather pointless. Newspapers have columns for a reason.


  • FoxDev

    @dcon said:

    My first thought looking at that (I do c++, not c#) is that temp's scope would just be in the call, like a for loop. That it extends beyond just seems wrong.

    In C#, scope is always to the innermost braces that contain the variable.
    @eskel said:
    What do your colleagues think?

    No idea, don't care; we're not using C# 6 yet anyway ;)



  • You avoid repeating the variable name, so it turns out shorter. And as the result of TryParse will often be immediately used and discarded, I see no issue in that.

    It's probably a good practice to limit the scope yourself to the if block when doing that, but meh, whatever.

    And it's also immensely helpful if you're not interested in the output parameter at all - for example, if you only need to check if a value is parsable. Unless C#6 finally supports optional output parameters...


  • FoxDev

    @Maciejasjmj said:

    Unless C#6 finally supports optional output parameters

    Not that I'm aware of



  • Yeah, that and no way to have a using declaration for enum values were always my biggest C# gripes.



  • @Maciejasjmj said:

    if you only need to check if a value is parsable.

    ... then use a function that takes the input as the only parameter and returns a boolean value.


  • kills Dumbledore

    @Evo said:

    good luck finding the declaration of "temp".

    highlight, f12

    @Maciejasjmj said:

    no way to have a using declaration for enum values

    That would require an enum to have a Dispose() method. Which doesn't make sense YR. If you want to limit the scope, you can brace a particular section (with appropriate comments so future maintainers don't rip it out)



  • The new syntax makes sense, I'd rather make the call 4 characters longer than add another line for a new variable that 90% of the time is throwaway.



  • Why not this:

    int temp; Int32.TryParse(messageString, out temp);
    

  • FoxDev

    Maybe it's just because it's new shiny, but I prefer the new syntax



  • Not the using block, the using statement at the top of the codefile, so that you avoid writing the enum name over and over, a la C++.


  • FoxDev

    There is a Static using coming in C# 6, but I'm not sure it applies to enums; it's main use seems to be

    using System.Console;
    
    public class Program
    {
        public static void Main(String[] args)
        {
            WriteLine("Where we're coding, we don't need System.Console!");
        }
    }
    

    Update: Based on this, it looks like it will apply to enums as well… which I'm not so sure about.


  • kills Dumbledore

    C#6 may have that. I know it has usings for statics, so you can have using System.Console; and then just call WriteLine()

    The other workaround is to use aliases to shorten them

    using t = ConsoleApplication1.Program.things;
    
    namespace ConsoleApplication1
    {
        class Program
        {
            static void Main()
            {
                t thing = t.thing1;
            }
    
    
            public enum things
            {
                thing1, thing2
            }
        }
    }
    

    this compiles fine


    Edit: partially hanzoed


  • As someone may have already pointed out, TryParse actually returns a bool, which if you actually understand the purpose of TryParse, you will use with an if statement, ternary or variable assignment to handle the success or failure. Combining any of these with the approach you have shown might start to look a bit silly.


  • FoxDev

    @SimpleSimon said:

    As someone may have already pointed out, TryParse actually returns a bool

    yeah. and i kind of wish it didn't. it would be nice if it returned a null on failure (making it return int? instead of int) so you could handle the error this way:

    // BUGBUG: C# doesn't actually work this way
    int temp = Int32.TryParse(messageString) ?? 0;
    

  • FoxDev

    I think the thinking behind returning a bool is so it's easier to use in if statements. But I can also see your version working well.



  • @accalia said:

    yeah. and i kind of wish it didn't. it would be nice if it returned a null on failure (making it return int? instead of int) so you could handle the error this way:

    // BUGBUG: C# doesn't actually work this way
    int temp = Int32.TryParse(messageString) ?? 0;
    ```</blockquote>
    
    ++
    
    And see if you can find the FTFY

  • FoxDev

    @RaceProUK said:

    I think the thinking behind returning a bool is so it's easier to use in if statements.

    true, but (1/3) * 10 * 3 % of the time the body of the if statement is just defining a default value. and you can still just as easily use it in an iff statement (did you get null? well then do a thing. :-P )

    @abarker said:

    And see if you can find the FTFY

    i've spent the last minute looking for it and i do..... oh. i @accalia'd in the comment of that code segment.



  • @accalia said:

    i've spent the last minute looking for it and i do..... oh. i @accalia'd in the comment of that code segment.

    Well that's not fair! You've fixed it in your post now, making it harder for other's to play! (Unless they look at your edit history, then it's easier).



  • Checking the edit history is more wortk than it's worth.


  • 🚽 Regular

    I didn't know whether TryParse came before or after nullable types, so I searched the webs and found this:

    Pavel Minaev [MSFT], 14 Aug 2012 2:12 PM
    The solution is to write your own extension method version of TryParse the way it would have been written had there been nullable value types available in the first place

    But Int32.TryParse (and the whole Try... pattern in general) was written when nullable value types were already available - it was added in .NET 2.0!

    Jon Skeet, 15 Aug 2012 9:51 AM
    The solution is to write your own extension method version of TryParse the way it would have been written had there been nullable value types available in the first place

    Nullable value types did exist when Int32.TryParse was introduced, in .NET 2.0. Any idea why they didn't go for it at that point?

    From the comments in this blog post.

    I assume these guys know are they are talking about. At least Jon Skeet is someone I trust to know a thing or two about C#.


  • FoxDev

    And from another post in those comments:

    TryParse came first. But seemingly only on Double :wtf:



  • @Evo said:

    C# is TRWTF.That syntax is terrible.

    Is it the out int temp which is objectionable? I had no idea that was even a thing you could do. Why would you even try to do that?



  • @RaceProUK said:

    useful when you need a throwaway variable.

    Every variable is sacred.



  • @Jaloopa said:

    highlight, f12

    Highlight is unnecessary, just plonk the caret on the search term (assuming Visual Studio?)



  • @Maciejasjmj said:

    Not the using block, the using statement at the top of the codefile, so that you avoid writing the enum name over and over, a la C++.

    TRWTF is having two completely unrelated meanings for the using keyword.



  • Wait, why does the variable defined inside an if statement have a scope larger than the if statement?



  • If you're talking about temp, it's defined inside Int32.TryParse() inside the if statement.



  • Yeah, so... why isn't the variable scoped to just inside the if?


  • FoxDev

    Because, in C#, scope is only defined by braces. Plus that syntax is sugar; it'd be a breaking change to the language if it followed different scoping rules 😜



  • @RaceProUK said:

    Because, in C#, scope is only defined by braces. Plus that syntax is sugar; it'd be a breaking change to the language if it followed different scoping rules

    Wait, so "for (int i = 0; i < 3; ++i) dosomething(i);" (I'm assuming that's valid C#, as well as C++) means i survives after the 'for'? If so, that's just wrong. If no, then the previous example is just wrong.


  • FoxDev

    The braces are implied for flow-control statements, and for scoping purposes, the flow-control statement itself is scoped to the (implied) braces. Same applies to the using block.

    I guess that technically counts as the exception that proves the rule 😆


Log in to reply