- Feature Articles
- CodeSOD
- Error'd
-
Forums
-
Other Articles
- Random Article
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
Frist!
Admin
TRWTF is that the ChangePosition method is probably an all-purpose utility tool which, in this instance, is being used to convert between US and European style date formats.
Admin
What kind of array has different data types at different indices? Well, object[], I suppose, but then you just have to remember index 5 is a DateTime, but index 1 is a bool? urgh.
Admin
And, by removing the entire conditional, Virginia introduced a bug. Obviously, ChangePosition has side effects that used to, sometimes, happen twice. Now they only happen once and everything breaks.
Admin
With your refactorings you introduced a bug in the whitespace program!
Admin
Why would it happen twice before?
Admin
That or that
m_ProgEnt.SAFF_SIGis a property and its getter does something ... more ... than one might expect, and now it isn't called.Admin
Sorry, but no. No matter what that function does or what the values of those variables are, the flow would lead to exactly one branch where only one function call would be made.
No, she probably introduced a bug by not knowing that the evil author of this code had redefined the operators == and != to do something slightly different at this specific code.
Admin
Right, your answer is a good way to see this end up breaking stuff too. And it's more possible because it doesn't have to be the product of an evil mind, just a moron which is far more common.
Admin
Are you sure? The second call to ChangePosition is outside the else branch. Am I missing something essential here?
Admin
If that's so, it's a good thing that this will break things as squirrelly hiding of side-effects is a Bad Thing in the first place.
Admin
If you think about it, maybe this throwing a bug would perhaps be a good idea -- because then the developer would be alerted to the fact that: ooh look, when I refactored this, that happened ... perhaps I'd better look at that pesky ChangePosition routine. Or the SAFF_SIG getter, etc. etc.
In fact, the developer had better take a look at the damn things anyway, to find out exactly what they do ... and down the rabbit-hole she goes ...
Admin
Had a very similar occurrence in a piece of embedded code. We thought the code did nothing so we removed it. The device then rebooted at seemingly random intervals. Turned out to be a timing problem and reinserting the original code made it work again. Since we didn't have the time to track down exactly what the timing problem was, we went with what worked (leaving a comment for the next author to ignore...)
Admin
A closer looks shows that the conditional are not the same. The second one has more spaces in it. Designed to space out the arguments in time to avoid that race condition the original dev was seeing.
(Do I need a /s on this forum? Or are you all smarter than the people over at Ars?)
Admin
The real WTF is the empty else block - rest of code seems to be a victim of refactoring.
Admin
"Sorry, but no. No matter what that function does or what the values of those variables are, the flow would lead to exactly one branch where only one function call would be made."
One call inside the if when
m_ProgEntis notnull, and one after the else part no matter whatm_ProgEntwas.Admin
"perhaps I'd better look at that pesky ChangePosition routine. Or the SAFF_SIG getter, etc. etc."
But why would I look at the getter? There is NO hint in the code that I deleted that there's a property in play. (In fact, this is the major difficulty I have with properties: they invoke programmer-generated code but look like they are just member variable accesses.)
But my general policy of NEVER using public data members in classes helps, because if there aren't any public data members, then it must be a property.
Admin
Isn't everything an array in PHP?
Admin
if (first) "frist" else "first"Could be something like this, you never know. Very subtle.
Admin
You'd look at the getter because the previous programmer deserves the benefit of the doubt for you to assume that their code isn't quite as atrocious as you currently believe it to be.
Also, this all might be something really sideways, like this current code-flow doesn't do much, but maybe there is an event watching the change position function that will trigger off of it. There are any number of ways that changing a variable can trigger other code to activate in ways that might be less than intuitive.
TRWTF isn't necessarily the code itself. IF the code isn't covering up something else, then the code is TRWTF. ELSE IF it is covering something else up, then the lack of comments is TRWTF, ELSE, it could be something completely different that is the TRWTF.
Admin
Wait, what?? OUCH! You're right... That's what I get for only skimming the code.
In my defense tho, a) it's not something I am tasked with fixing, so I allowed myself to not be so thorough; b) I was kinda in a rush to go back to work; c) I was low on caffeine and I had a headache at the moment I read the OP.
Admin
The author of the code is fucking sick. If you write your own project that nobady else will touch, then screw it. Do what ever sick thing you want (but still I don't think this is a good reason to do such stuff in your code). But when you work in a team, please, STOP YOUR SICK MIND from doing job in the least obvious ways because you write code NOT FOR YOURSELF but the other peaple in your team (and the people who will support your shit after you leave company). The code above is only few lines of code (without curly braces) and it has a lot of shit: Why the hell the geter has side effects (and duplicated code on both branches)?Create a fucking method, and do call it before 'if' staetment Why the hell, the name of the variable is 'ar' are the all of the meaningfull names are already used? Why the hell, the indexes are not moved into a named constant?
Why not create a structure (or class, or whatever your language has) with human-readable fields to hold your data. Then you don't need to rememeber the index of the object you need, the type of the object you need, don't need to cast every time.
Why, why do people do things in so unobvious? Why not make things more simple?
Admin
And that's how you create a WTF...
Admin
It's a regrettably-common practice, unfortunately. I suspect that the mentality behind it is something along the lines of "I could create a class for this, but it's only used in one place and I'd have to think about where to put it... let's just return an array."
Admin
Every so often I encounter something similar in a 20+yo codebase I maintain. It sometimes comes down to (a lack of) YAGNI when writing, but more often comes down to "YNLNI" (You no longer need it).
Admin
I thought the array might be a variable arguments array so you can call the method with a variable number of parameters. If so, I would hate to see the rest of that method. I wouldn't be surprised if ar[1] is only a DateTime if the number of arguments was 7.
Admin
On an unrelated note, can we get a button to display hidden comments that are "Held for moderation". We can consider it a warning, and take a peek anyway.
Admin
I just assumed that was an editing error and that that last one was actually in the outer else, but I guess anything is possible.
Addendum 2017-10-12 01:49: Who knows, maybe that fixed a subtle bug caused by the statement sometimes running twice...
Admin
"Why the hell the geter has side effects"
We don't see the getter in this snippet, so we don't know if it has side effects, BUT we also know that the number of getters out there that do have side effects is NOT zero, so we have to look at it to see. But we don't see any visual clue that there is a getter, so either we assume the other programmer is a fucknut (high probability, given the code we see here), or we get burned because we gave him the benefit of the doubt and it turned out that he was, indeed, a fucknut.
Admin
Always assume the other one is a fucknut!
Admin
It's called "defensive coding", and it's as efficacious a technique as "defensive driving" -- always be prepared for the other person to be stupid.
Admin
Especially when the other programmer was you 6 months ago.
Admin
Especially then :)
Admin
I was given a task to improve response time for a Customer Orders system. Found and fixed a lot of crap. The worst offender was a getter that did a crap load of I/O against a slow archaic proprietary (from back in the DOS days) database. The getter was for a field called CustomerBalance. It added up all outstanding Invoices. There was already a Customer Class initialized for the Customer. The lazy bum who created it couldn't even be bothered to create and populate a private local for this new field. He simply tossed everything right in the getter. Over time, people assumed (who wouldn't) CustomerBalance was the same as anything else: Name, Phone, Address, etc. Anytime they needed a balance they'd just reference it. Well I traced a sample Retrieve, CreditCheck, Allow new Order process from start to finish. There were 7 references to CustomerBalance at 3 seconds apiece and people wondered why Ordering was so damn slow.
Now visualize the getter doing updates as well and returning something based on the Updates. The next time you call it you get a different result. Perhaps that's what they needed to happen on the second invocation of ChangePosition().
NEVER trust a Get/Set. Anyone can throw anything they want in there.
Admin
Bonus points if the only purpose of
m_PSOC_SIGwas for that conditional statement.Admin
No, you don't write code for other developers, you write code to accomplish some business process in the most efficient way possible. Sorry, I had enough of constantly hearing "don't use feature X, developer Y doesn't understand it" and what that kind of developers thinks (or doesn't) isn't important.
Also, I guess there's no article for today.
Admin
If m_ProgEnt, if ChangePosition isn't idempotent, won't the "fix" break the behavior?
Admin
I'd say I felt cheated but, given the recent efforts, all I've missed out on is five minutes of wasted life.
Admin
OK, OK! We get it. A nefarious coder chose to break away from conventional wisdom, while we have an implicit expectation foreach member evaluation to do not an unsafe override, but to behave like a default backing field. We should continue to be vigilant, in case the event ever arises again. Do you not long for a typeof language which forbids you to alter its very fabric, lest you finally yield to your volatile whims out in public?
Admin
At least with driving you don't have to worry about colliding with yourself from six months ago. (Only about colliding with a future version of yourself from after time travel is invented.)
Admin
I disagree. No matter how beautiful and perfect your code is at the moment you release it, it will at some point require changes. Maybe the business process is different, maybe the technology stack is being updated, maybe the code is now being used in a way you never anticipated. At that point someone is going to have to work with your code. And the easier you can make it for them, the more value you will bring to your organisation.
Admin
Tell me! Writing a shitty properties (or whatever else your 'business processes' requires to accomplish) that force you to duplicate statements inside 'if' and 'else' branches - yeah, that's the real 'feature X' for the real programmers, not for the pussies, scared of mutability and global states. Good luck in your accomplishing of business processes, sir.
Admin
I want to see the comments that are being held for moderation! I am a big boy so I can handle it so BRING IT !!!!