• Frist (unregistered)

    Frist!

  • Quite (unregistered)

    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.

  • Menno (unregistered)

    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.

  • djingis1 (unregistered)

    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.

  • Whitespace programmer (unregistered)

    With your refactorings you introduced a bug in the whitespace program!

  • Guest #1287556 (unregistered) in reply to djingis1

    Why would it happen twice before?

  • Steve_The_Cynic (nodebb) in reply to djingis1

    That or that m_ProgEnt.SAFF_SIG is a property and its getter does something ... more ... than one might expect, and now it isn't called.

  • Ex-lurker (unregistered) in reply to djingis1

    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.

  • Ex-lurker (unregistered) in reply to Steve_The_Cynic

    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.

  • MacFrog (unregistered) in reply to Ex-lurker

    Are you sure? The second call to ChangePosition is outside the else branch. Am I missing something essential here?

  • dkf (nodebb) in reply to Steve_The_Cynic

    its getter does something ... more ... than one might expect

    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.

  • Quite (unregistered) in reply to Ex-lurker

    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 ...

  • sam (unregistered)

    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...)

  • Brian Boorman (google)

    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?)

  • CodeMonkey (unregistered)

    The real WTF is the empty else block - rest of code seems to be a victim of refactoring.

  • Steve_The_Cynic (nodebb) in reply to Ex-lurker

    "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_ProgEnt is not null, and one after the else part no matter what m_ProgEnt was.

  • Steve_The_Cynic (nodebb) in reply to Quite

    "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.

  • You've not heard of PHP? (unregistered) in reply to Menno

    Isn't everything an array in PHP?

  • herby (nodebb)
    if (first) "frist" else "first"

    Could be something like this, you never know. Very subtle.

  • Kashim (unregistered) in reply to Steve_The_Cynic

    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.

  • Ex-lurker (unregistered) in reply to MacFrog

    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.

  • Big fat arrow (unregistered)

    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?

  • DQ (unregistered) in reply to Ex-lurker

    And that's how you create a WTF...

  • Simon (unregistered) in reply to Menno

    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."

  • Gerry (unregistered)

    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).

  • Chris (unregistered)

    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.

  • Chris (unregistered)

    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.

  • Scarlet_Manuka (nodebb) in reply to Steve_The_Cynic

    one after the else part no matter what m_ProgEnt was

    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...

  • Steve_The_Cynic (nodebb) in reply to Big fat arrow

    "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.

  • DQ (unregistered) in reply to Steve_The_Cynic

    Always assume the other one is a fucknut!

  • Quite (unregistered) in reply to DQ

    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.

  • JG (unregistered) in reply to DQ

    Especially when the other programmer was you 6 months ago.

  • DQ (unregistered) in reply to JG

    Especially then :)

  • Appalled (unregistered)

    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.

  • anonymous (unregistered)

    Bonus points if the only purpose of m_PSOC_SIG was for that conditional statement.

  • Zenith (unregistered) in reply to Big fat arrow

    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.

  • Mike (unregistered)

    If m_ProgEnt, if ChangePosition isn't idempotent, won't the "fix" break the behavior?

  • MiserableOldGit (unregistered) in reply to Zenith
    Also, I guess there's no article for today.

    I'd say I felt cheated but, given the recent efforts, all I've missed out on is five minutes of wasted life.

  • Ulysses (unregistered) in reply to Appalled

    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?

  • gcewing (nodebb) in reply to JG

    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.)

  • jkshapiro (nodebb) in reply to Zenith

    You don't write code for other developers, you write code to accomplish some business process in the most efficient way possible.

    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.

  • Big fat arrow (unregistered) in reply to Zenith

    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.

  • Dr. λ the Creator of Variables, Binder of Variables, Applicator of Terms and β-Converter of Redexes (unregistered)

    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 !!!!

Leave a comment on “Refactoring the Conditional”

Log In or post as a guest

Replying to comment #:

« Return to Article