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.
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);
-
int temp;
Int32.TryParse(messageString, out temp);int differentReturnInteger = GetReturnIntFromDropdownID(temp);
Nice handling of error cases from that
TryParse
…
-
```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 toDropdowns_ID
... It looks like nothing would work if that wasn’t the case.
-
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.
-
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 ...
-
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 toGetReturnIntFromDropdownID
, and abarker just confirmed my fears that there is no Dropdown_ID of 0.
-
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
...
-
Yeah, if you don't mind having no control over the exception thrown or its contents…
-
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.
-
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.
-
What's so terrible about it?
-
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'scaller's face.
-
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.
-
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.
-
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.
-
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.
-
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...
-
-
Yeah, that and no way to have a
using
declaration for enum values were always my biggest C# gripes.
-
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.
-
good luck finding the declaration of "temp".
highlight, f12
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);
-
Maybe it's just because it's new shiny, but I prefer the new syntax
-
Not the
using
block, theusing
statement at the top of the codefile, so that you avoid writing the enum name over and over, a la C++.
-
There is a Static
using
coming in C# 6, but I'm not sure it applies toenum
s; it's main use seems to beusing 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.
-
C#6 may have that. I know it has usings for statics, so you can have
using System.Console;
and then just callWriteLine()
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.
-
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 ofint
) so you could handle the error this way:// BUGBUG: C# doesn't actually work this way int temp = Int32.TryParse(messageString) ?? 0;
-
I think the thinking behind returning a
bool
is so it's easier to use inif
statements. But I can also see your version working well.
-
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 ofint
) 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
-
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 )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.
-
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.
-
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
From the comments in this blog post.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?
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#.
-
And from another post in those comments:
TryParse
came first. But seemingly only onDouble
-
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?
-
-
highlight, f12
Highlight is unnecessary, just plonk the caret on the search term (assuming Visual Studio?)
-
Not the
using
block, theusing
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 insideInt32.TryParse()
inside theif
statement.
-
Yeah, so... why isn't the variable scoped to just inside the if?
-
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
-
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.
-
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