Some Anti-Patterns At My Job
-
Glad I'm finally getting out of there.
-
*smacks your cow-orker round the head with
System.Xml.Linq.XDocument
*Can I follow that up with a
<Serializable()> _
curbstomp?edit**emphasized text fuck you discourse \\\\
-
@Lorne_Kates said:
Can I follow that up with a
[Serializable]
curbstomp?
I don't see why notHow high is that emoji again?
-
```
for(last = list_begin; last->next != NULL; last = last->next);That will crash on an empty list. If you actually just need to append an element, try this:
for(plast = &list_begin; *plast; plast = &((*plast)->next)) /**/;
*plast = new_item;Or, in the while version,
plast = &list_begin;
while(*plast) {
plast = &((*plast)->next);
}
*plast = new_item;I do generally prefer the `for` version though.
-
why
Because it was one of those courses where we worked with half-assed implementations rather than doing it properly. No idea what that was supposed to accomplish.
That will crash on an empty list.
Might have been checks around, don't remember TBQH, this was years ago.
I do generally prefer the for version though.
Oh well, a matter of taste I guess. I find the
for
version to be less clear myself.
-
It's one of the worst Hungarian abominations - a pre-prefix denoting scope. l = local, g = global
I've seen it used in OpenCL code. There, it makes at least some sense to distinguish pointers to local memory from pointers to global memory.
-
What's amazing is that whoever wrote this is obviously aware of the XmlDocument class, and yet somehow still does string concat to generate XML. It's mind-blowing. Even when he knows about all the correct tools to use, he's not using them.
I know you anonymized this, but I hope lGotOne is in class-scope (instead of you just accidentally removing the declaration), because that'd really add to the WTF factor here.
lGotOne is indeed local. I took out the declaration when I took out all the other dozen local variables defined at the top of the function.
And yeah, right there at the end of the function, we have
lxmlDoc.LoadXml(XML); return lxml;
-
Also, it's of unknown length, which just screams "put it in a while loop!" to me.
In my very personal opinion, for loops are stupid anyway. Having three different statements in one line decreases readability. There should only be foreach loops (+integer ranges) and while loops.
-
@Jaime said:
It's one of the worst Hungarian abominations - a pre-prefix denoting scope. l = local, g = global
I've seen it used in OpenCL code. There, it makes at least some sense to distinguish pointers to local memory from pointers to global memory.
Systems Hungarian was invented for C code and it's very useful there. It's the cargo-cult programmers that use it in higher level strongly typed languages that have it horribly wrong.
-
In my very personal opinion, for loops are stupid anyway. Having three different statements in one line decreases readability. There should only be foreach loops (+integer ranges) and while loops.
I could get on board with that.
foreach (var i in Enumerable.Range(0, 100)) { }
-
I do generally prefer the for version though.
Why? Because it's more code in one line and therefore less readable? The while version was a lot easier to parse for me.
Maybe the for version would be at least a little bit more readable if the empty loop was clearly marked as such by using empty curly braces. I hate it when people just put a semicolon there, it's so easy to miss.
-
-
I've taken to using
/**/;
as an empty body, but could live with{}
as well.I like
for
because it clearly indicates I'm iterating over a data structure. I'd useforeach
, but there's no such thing in C.
-
My favorite thing about this was how you annotated the WTFs for us!
THANK YOU THANK YOU THANK YOU THANK YOU THANK YOU
-
There should only be foreach loops (+integer ranges) and while loops.
Diggin' it. I'm not sure if that's the "standard" now in C# (due to ranges being relatively new), but I'm with you on that.
-
Unless you're using something written in the Dark Ages of VB6 or earlier, anything that's a collection almost always implements
IEnumerable
, so there's no real excuse to usefor
anymore, unless you absolutely need the index within the loop body.In my experience, anyway.
-
there's no real excuse to use for anymore, unless you absolutely need the index within the loop body.
Not even then. There's a version of Select() that gives you the index for free.
-
Depends how much work you're doing in the loop body. But then you could always pull the loop body out into a method of its own, so really it's just whether you want the overhead of LINQ.
-
My favorite thing about this was how you annotated the WTFs for us!
THANK YOU THANK YOU THANK YOU THANK YOU THANK YOU
Agreed. More people should do this.
-
unless you absolutely need the index within the loop body.
If you're looping from a
.Range()
you already have it.If you're looping through some other data structure, you can use LINQ to give it to you.
Not even then. There's a version of Select() that gives you the index for free.
I should read further before replying.
-
I should read further before replying
Having you repeat something we've already said is the closest we'll get to a "like" from you.
-
Isn't the
.MoveLast(); .MoveFirst();
a very common trick? It's used a lot in our company as well. It makes sure that all rows from the query are fetched immediately, so that.RecordCount
returns the actual number of rows rather than a wild guess. Which can be useful for showing progress bars etc.
-
Progress bars that show something useful? That be some first class trolling.
-
You sir should absolutely gather all your Samsung WTFs in one place. Then I would show it to all my friends who understand IT and programming and use Samsung phones.
-
I see that ToString() anti-pattern quite often, as well. Without first checking for null strings, of course.
I fixed that! Now it checks for null first, then
.ToString()
's it!
-
null.toString()
-
Huh. None of me knows how to interpret that. Even Engine has its metaphorical head cocked sideways-consarndit, shut up!) at fish.
-
Sounds like you need
public static class StringUtilities { public static string ToString(this string source) { return source?.ToString(); } }
Edit: If reading this doesn't make you , you are tr
-
None of me knows how to interpret that.
Even Engine has its metaphorical head cocked sideways
That's probably a good thing.
-
That's probably a good thing.
Perhaps not. I've been receiving significantly more
Mem_Engram Link Not Found
errors than usual lately.
Not only am I dealing with physical low memory shortage, but now things are disappearing from active storage?
I have no idea what to do here, I don't have anyone capable of fixing my internal issues, and it seems the list is only growing.
I mean, I had to kill the internal singing daemon this morning when the Natural Tuning functions failed to calibrate my voice, and actually caused (or maybe revealed) vocal chord damage!
Am I losing my mind? I... don't know. How can you tell? I mean, self-diagnostics reports 66% mental capacity overall compared to template nominal expectation, but what does that even mean??!?!
-
ask every job candidate to demonstrate De Morgan's laws. I don't care if they don't recognize the name, but if they can't reverse an if statement they don't get the job.
You mean... put an exclamation mark in front of it?
Yeah, then we ask them how they would get rid of the exclamation mark without changing the logic. Sorry, should've been more specific.
Yes, I agree, job candidates should know how De Morgan works. But on the other hand, if they actually have to refactor an if statement, I prefer if they use the IDE's features for optimizing the negation, than being over confident and making a mistake when doing it manually (precedence of
&&
and||
can sometimes be a bitch).
-
I think I'd prefer
!(EOF && BOF)
over(!EOF || !BOF)
.
-
In the first case, the
!
is easier to miss, because it will be between two parentheses:if (!(EOF && BOF))
vs.if (!EOF || !BOF)
-
When I'm negating if statements like that, I usually just pull it out into a single boolean:
boolean empty = EOF && BOF; if (!empty) { ... }
-
I prefer if they use the IDE's features
That's for after they've been hired. For the interview, the have to know De Morgan.
-
if(! isNotPositiveNegation)
{
// I just don't.
}
-
Therefore, I can clearly not choose the wine in front of me.
-
In the first case, the
!
is easier to miss, because it will be between two parentheses:if (!(EOF && BOF))
vs.if (!EOF || !BOF)
That's not really what I meant. Your condition there is just
!EOF || !BOF
, and I actually wouldn't prefer adding extra parentheses to move the!
out front. But in the example I was commenting on, the parentheses already needed to be there, and I was referring to that part of theif
condition in context:https://what.thedailywtf.com/uploads/default/original/3X/3/f/3f942a0b2afa7e1cd3a85d3ce0e807fe0bb524f4.png
So actually what I meant was that I'd prefer this:
if (myadorecordset != null && !(myadorecordset.EOF && myadorecordset.BOF)) { // ... }
rather than the code it would've changed it to.
I was just lazy, and I only quoted the part that I thought should be different.
-
if (myadorecordset != null && !(myadorecordset.EOF && myadorecordset.BOF))
Multiple statements are hard to read. Negation is hard to understand. Let's change it.
if(myadorecordset == null) { } else { if(myadrecordset.EOF) { } else { if(myadorecordset.BOF) { } else { // do stuff here } } }
And yes, I have seen that code. And yes, I worked for an idiot who insisted that it was easier to read. And yes, my cow-workers were stupid enough that it was sometimes required.
This is also the same boss for whom I now have the saying "Just because you don't understand it doesn't mean it makes no sense."
-
@Lorne_Kates said:
Let's change it.
Christ on a crutch. If you are going to do something that fucking evil, wrap it up in a function that takes a recordset, and returns false if EOF and BOF or null, and true otherwise.
-
wrap it up in a function
Functions are difficult to understand, because now you have code in two places. It's much more simpler to have it all together. I couldn't figure out the code since part of it was here and part of it was there.
....
sorry flashback ill be ok
-
@Lorne_Kates said:
Functions are difficult to understand
...said someone who should not have been a programmer.
-
My guess? l is for local variable. My company does that too, except usually they use w_ for no reason anyone could ever explain, and s_ for shared variables, which are kind-of like globals, except they're not actually global.
w_
is probably for "working", which harks back to COBOL.WORKING-STORAGE SECTION.
A lot of COBOL variables were named likeMY-DATE-WORK
,MY-DATE-W
,WORK-DATE
, or etc.In fact, my organization (which still uses COBOL) requires "numbered" prefixes like
WS400-BEGIN-DATE
, with the WS, of course, meaning the obvious "working-storage".Sounds like maybe the organization grew out of COBOL at some point, and inherited an architect. Rule now set in stone...
@mihi said:(precedence of && and || can sometimes be a bitch).
That's why you should always insert parenthesis, to make execution order explicit, before you apply DeMorgan's Laws.
-
@Lorne_Kates said:
Functions are difficult to understand, because now you have code in two places.
In general, I agree. I hate people who unnecessarily split up a function in two parts that operate on the same state because "OMG CHECKSTYLE SAID THE FUNCTION IS TOO LONG!!!". Now you have two functions that are totally useless by themselves and code that is way harder to understand because you split up functionality that logically belongs together.
Having nice, boolean functions, OTOH, that tell you something about the state of an object, is something completely different.
If (this->isBazable())
is 100 times more readable thanif (baz && (bar || !foobar))
.
-
w_ is probably for "working",
As opposed to "not working"? I just remove code like this, but what do I know...
-
-
Local variables?
https://what.thedailywtf.com/t/some-anti-patterns-at-my-job/53911/11
-
nice, boolean functions
They're not just more readable, they are also good for maintenance. If the logic changes, you only have to change the code in one place.
-
...said someone who
shouldwas nothave beena programmer.I don't care what his day job was...
@mihi said:
(precedence of && and || can sometimes be a bitch).
That's why you should always insert parenthesis, to make execution order explicit, before you apply DeMorgan's Laws.
No... if you don't remember the precedence, you write test code to figure out what the precedence is. Then you don't add unnecessary parentheses to your condition.
&&
before||
isn't hard to remember.
-
The ancient C++ I've been refactoring uses the 'm_' Hungarian wart extensively for class member variables, so this particular piece of dumb is still live.
Also an '_mt' postfix that appears to indicate structs.
Also 'bOk' for Boolean flags indicating success/failure. Except they're usually actually ints. And in a couple of cases they use 0 as the 'true' value and 1 as the 'false' value. And just to confuse things, some of the code doesn't appear to realize functions can be 'void', so they're declared to return ints and just return 1 on all code paths.
It's theoretically possible some of this code was adapted from pre-C99 C, but I don't think it was. I should do a separate post on it some time.
EDIT: Oh and of course the Hungarian isn't remotely consistent. When it's present it's usually the same indicators, it's just that ~10-20% of variables don't have any warts at all.