Chubertdev's snippet thread
-
I hate it too, but it's a byproduct of "compatibility".
Personally, I purge it everywhere I can when I find it, even if VB is being helpful and effectively giving you a "free" variable (named the same as the function) and returning it at the end for you instead of you having to do it manually.
Which further spreads the "return at the end" anti-pattern.
-
And a variable you have to be a little bit careful with how you use it lest you make the function recursive without meaning to.
Is there any other language where you can accidentally make a function recursive?
-
```
If Method(Arguments) Then
...
Else
Throw New Exception("blah blah blah")
End IfBecause throwing the exception from inside the method would be too smart.</blockquote> What's wrong with that? I think it's missing context, because a method could definitely exist to test a condition which is not necessarily an error, but is in this particular function. The only issue I see with this code is not throwing the error in the body of the if, like: if not Method(Arguments) Then Throw New Exception("Blah") ...
-
It's the only way that it's called, so it's a single context.
But you are right in that if it were used differently, it would make sense.
-
Refactoring gone wrong:
For _RecCount As Integer = 1 To 1 '_MaxImageCount
-
Oh boy.
'TODO: trade server controls for HTML controls to save memory
-
Whoa.
For i As Integer = 0 To objDataSet.Tables(0).Rows.Count - 1 Select Case i Case 0 If Not IsDBNull(objDataSet.Tables(0).Rows(i).Item("RecordID")) Then RecordID1 = CInt(objDataSet.Tables(0).Rows(i).Item("RecordID")) Case 1 If Not IsDBNull(objDataSet.Tables(0).Rows(i).Item("RecordID")) Then RecordID2 = CInt(objDataSet.Tables(0).Rows(i).Item("RecordID")) Case 2 If Not IsDBNull(objDataSet.Tables(0).Rows(i).Item("RecordID")) Then RecordID3 = CInt(objDataSet.Tables(0).Rows(i).Item("RecordID")) End Select Next
-
With every edit, that snippet just gets more and more confusing.
-
Yeah, I kept adding more detail to it. I have one more edit, this one should put the cherry on top.
-
It just turned my brain off.
Glad it's quitting time anyways.
-
Basically, I cleaned up the method, then reverse engineered my edits. One at a time.
Final version:
If Not IsDBNull(objDS.Tables(0).Rows(0).Item("RecordID")) Then RecordID1 = CInt(objDS.Tables(0).Rows(0).Item("RecordID")) If Not IsDBNull(objDS.Tables(0).Rows(1).Item("RecordID")) Then RecordID2 = CInt(objDS.Tables(0).Rows(1).Item("RecordID")) If Not IsDBNull(objDS.Tables(0).Rows(2).Item("RecordID")) Then RecordID3 = CInt(objDS.Tables(0).Rows(2).Item("RecordID"))
-
Refactoring gone wrong:
For _RecCount As Integer = 1 To 1 '_MaxImageCount
Ok, then.
For _Count As Integer = 0 To 0
-
a method could definitely exist to test a condition which is not necessarily an error, but is in this particular function
I would probably wrap the function in a function called "validateSomethingSomething" so I can throw from the validation function. It's obvious from the name what the validation function is intended to do and moving the "if" and the "throw" declutters the function making the call, hopefully making that function's purpose much clearer too.
-
wut?
currentUser[attr] = (/^true$/i).test(currentUser[attr]) ? true : (/^false$/i).test(currentUser[attr]) ? false : currentUser[attr];
-
Dim li As New ListItem
I remember this. In classic VB, didn't this put
If li Is Nothing Then Set li = New ListItem
before every single reference to li?
Not only is it inefficient, it totally breaks stuff like the code to deal with ADO that folks were talking about up-thread.
-
Yeah, I'm lucky that the only VB I deal with these days is .NET
-
C#:
var startupScript = new StringBuilder(); startupScript.Append(string.Format("sValue = '{0}';", strValue)); Page.ClientScript.RegisterStartupScript(Page.GetType(), "startupscript", startupScript.ToString(), true);
-
I'm envious. I still get dragged back into the VB6 / VBA world from time to time, though thankfully no one round here is trying to do real time machine control with it anymore.
-
wut?
Booleanify a string, but only if it's actually a boolean. Problem?
-
This inspires a lot of confidence.
Dim iFooterStartBottomY As Integer = 727 'was 720
-
I wish that this snippet could be optimized.
switch (event.keyCode) { case 13: // enter key CheckState(bRefresh); }
-
Professional.
<input type="button" id="btnDoStuff" value="Search" style="width:120px" onclick="javascript:CheckState(true);"/>
-
I don't see how you can optimize that. Refactor it, sure, but switch with one case is the same as an if statement in speed.
-
Arrays of charsBits are precious.
-
If source code size is an issue, remove all the comments and whitespace.
-
JavaScript:
switch(true) { case /^checkbox/.test(control.type): // ... case /^text/.test(control.type): // ... }
Not necessarily bad, it definitely works, but why structure it that way?
-
that.... that works‽
ICKY BAD NO! Match your braces please!
:do_not_want.avi:
-
Yeah, had one extra character.
-
why structure it that way?
As a wild guess, maybe the author anticipated having a bunch more cases, and thought that was cleaner syntax than if ... else if ... else if ... else if ... else if ...
-
As a wild guess, maybe the author anticipated having a bunch more cases, and thought that was cleaner syntax than if ... else if ... else if ... else if ... else if ...
If this
switch
handled that many types of HTML controls, there are much bigger problems...
-
The funny thing is that it'd be cleaner to do something like
switch(control.type)
-
If this switch handled that many types of HTML controls, there are much biggere problems...
Fair enough, but insufficient context was supplied to know that.The funny thing is that it'd be cleaner to do something like switch(control.type)
Yes, well, we already know from the fact that you are posting these snippets here that the author wasn't the brightest bulb on the Christmas tree.
-
Fair enough, but insufficient context was supplied to know that.
How so? It's testing for
checkbox
andtext
by evaluatingcontrol.type
, I think it's pretty obvious what types of objects it's looking at.
-
I meant it wasn't apparent from that little snippet that handling additional types of controls would be indicative of bigger problems. There is no information about what it's doing with them, nor why. Also, I probably should have written that the context was insufficient for me, a totally not JS person, to know that.
-
Today's annoying little snippet:
this.regData.setKeepMeInformed((this.props.getProperty( KEEP_ME_INFORMED_PROPERTY_NAME).equals(TRUE) ? true : false));
Because we love
this
, extra parentheses are fun and you can't trust an equality test to give a boolean answer…
-
There are worse things:
if (this.props.getProperty(KEEP_ME_INFORMED_PROPERTY_NAME).equals(TRUE)) this.regData.setKeepMeInformed(true); else { this.regData.setKeepMeInformed(false); }
I've seen that quite a few times in programs I'm working on.
-
sadly i can one up you there.
if (this.props.getProperty(KEEP_ME_INFORMED_PROPERTY_NAME).equals(TRUE)){
this.regData.setKeepMeInformed(true);
else
this.regData.setKeepMeInformed(false);
}if (this.props.getProperty(this.meta.props.names.KEEP_ME_INFORMED).equals(this.meta.consts.names.TRUE)){ this.regData.setKeepMeInformed(this.meta.consts.TRUE); } if (this.props.getProperty(this.meta.props.names.KEEP_ME_INFORMED).equals(this.meta.consts.names.FALSE)){ this.regData.setKeepMeInformed(this.meta.consts.FALSE); }
let's just say that code no longer exists in my codebase... and i had a nice long curse at the now departed developer that wrote it....
-
I particularly like how you've got those interesting decorative
{
…}
to elevate it from mere Representative Line to full-on CodeSOD material (or is that a typo? ;))
-
Sorry, fixed it!
-
Sorry, fixed it!
not quite... still missing one}
nevermind, but you did add another WTF...
i know those braces are technically optional when the conditional is a single line... but i've got caugnty with too many bugs because of it. my linting rules always make it an error to omit the braces...
-
There are worse things:
if (this.props.getProperty(KEEP_ME_INFORMED_PROPERTY_NAME).equals(TRUE))
this.regData.setKeepMeInformed(true);
else {
this.regData.setKeepMeInformed(false);
}I've seen that quite a few times in programs I'm working on.
I've seen that antipattern before where it was justified by the fact that the codebase was huge, had about 30 developers, multiple live versions of the code, and so many changes that people were not allowed to make unnecessary changes; the two branches used to do something different, but were changed like that. If you collapsed down the if statement, you'd cause extra lines to appear in a diff, and that caused extra work for the build master.
-
Uhh...
SET @EndDate = DATEADD(ms, -3, DATEADD(dd, 1, @StartDate))
-
I like how this isn't even a client-side comment, nevermind removed like it should have been.
<%--<link href="styles/reset.css" rel="stylesheet" type="text/css"/>--%>
-
Is that the only server-side code in the entire file? Please say it is.
-
It is*
* - no, it isn't
-
seen today in a C# class written by one of my coworkers:
string op = (string)this.payload.Operation;
not that's not too much of a WTF until you realise that:
- this is the only line in the whole class that doesn't decalre variables as
var
(honestly i don't care if you use var or not, just be consistent!) - there is no local variable payload that needs the this keyword to unmask the global instance
- Operation is already strongly types as....
string
so the cast is a noop
- this is the only line in the whole class that doesn't decalre variables as
-
Clearly, if we search until 2 ms before 24 hours of the start of the range, we'll have gone way too far.
-
Operation is already strongly types as.... string so the cast is a noop
I have no idea why, but people love to do that. I want to override
String.ToString()
to throw an exception.
-
It's even worse than that. People often use .ToString() as defensive coding when something is declared as System.Object, but is supposed to contain a string, so that they don't get a run time exception. In the few cases where the .ToString() isn't a no-op, it returns the class name and is almost never what you want. This pattern turns runtime errors into bad output, which does nothing but make them harder to find.
-
It's even worse than that. People often use .ToString() as defensive coding when something is declared as System.Object, but is supposed to contain a string, so that they don't get a run time exception. In the few cases where the .ToString() isn't a no-op, it returns the class name and is almost never what you want. This pattern turns runtime errors into bad output, which does nothing but make them harder to find.
You could almost say that's.........Worse Than Failure