• RussellF (unregistered)

    Correction: the "Stock" property doesn't indicate whether the part's in stock, it's a flag that governs how and when it's re-stocked.

  • (nodebb)

    I'd have gone for putting the terms of the simplified version in the opposite order, just in case there's something significant to the test ordering. (I really hope that the Stock property is trivial in its implementation, because if it isn't, this is all a massive screaming pile of WTF.)

  • RussellF (unregistered) in reply to dkf

    All of these fields are just reporting the value of a private variable, no computation involved.

  • DrPepper (unregistered)

    <quote>A few days ago I was assigned to a task where I couldn't remember exactly where the code was that I'd need to look at. However, I did remember that I'd submitted some of it to TDWTF, so I did a search of your site for my submissions to find it</quote>

    This is the funniest thing I've read all week.

  • Carl Witthoft (google)

    Let me create my own WTF here by assigning bits in a byte. bit_0 : part.Stock bit_1: part.CloseOut bit_2: part.IsAvail , and so on if other situations pop up. Now all ya need to do is look at the decimal value of said byte and you know the truthiness of "Can the customer order?"

  • MaxiTB (unregistered) in reply to RussellF

    Interestingly enough, Roslyn is actually pretty good with optimizing boolean expressions, so the CIL will end up to be the same after the compilation as written above. However for clean code purposes, it's never a good idea to over complicate expressions in general and you should never rely on the compiler to do the work for you, because in many situations it prefers the more safe option.

  • (nodebb)

    Honestly I'd rather have the business logic spelled out explicitly.

    Addendum 2022-04-12 12:23: That is, the same way that the stakeholders defined it. If you get sign off on the simplified version, great.

  • mraqndernuucl (unregistered) in reply to konnichimade

    +100, what if down the road only one of those three conditions is removed? With the original code, we can trivially delete exactly that, with the "improved" code someone has to spend significant time and effort to figure out how the documented conditions map to the expression.

  • WTFGuy (unregistered)

    Agreed again.

    The developer's job is to acurately implement the requirements. The closer the code corresponds, the easier the code validation. Heck the unit test plan for this function should have one example and one counter example for each of the three passing "can order" cases. Test like you code and code like you test.

    Let the optimizer reason about deMorgan's Laws as it may. Not your job. Has't been for 30 years now.

  • Nick (unregistered)

    I’m going to throw out a dissenting opinion. I don’t want to have to reason through the business logic when debugging code - I’d much rather have that simple OR than have to consider three OR’d ANDs…

    However, when writing the unit tests, I would definitely want to spell out those three conditions. Unit tests should match the business logic - but the implementation should be the simplest possible code that causes all the tests to pass.

  • Tinkle (unregistered)

    So the real WTF is the specification is needlessly complex?

  • Mike Sandbox (unregistered) in reply to Nick

    For reasons other people have put forth already, my preference would be to keep the original logic but add comments and formatting it so it's more clear.

    //This could be simplified but the business rule specifies these exact cases if (and-condition-1) //Requirement 1 || (and-condition-2) //Requirement 2 || (and-condition-3) //Requirement 3 {

    }

  • (nodebb) in reply to Nick

    Unit tests with the business logic spelled out definitely make this more malleable.

  • (nodebb) in reply to WTFGuy

    To be fair, as I mentioned in my addendum, if you apply de Morgan's laws and realize the business requirements are redundant, you can always bring your minified logic back to the client and say, "Look, those rules you provided actually boil down to [a || !b]." That's a great opportunity for everyone to get onto the same page.

  • (nodebb)

    It makes a lot of sense to make it easy to distinguish between properties that correspond to actual database fields and properties that are calculated on object initialization. You can't use the later in LINQ queries operating on IQueryables (transformed to SQL), unless you do some clever transformations before handling the expressions to Entity Framework or whatever library you use to access the database. I don't use different casing for those (mostly because I do have those transformation in place in those cases I care about) and I have been bitten many times by a using property that exists only on the .Net side in an expression that later failed to get transformed to SQL.

  • (nodebb)

    I disagree. The "Expanded form" is very clear as to what the requirements are *as communicated to the developer". If there had been conversations and the requirements were changed to the concise form, then the concise code would match..... Given the rewritten code, what is to be done when 5 years from now a memo arrives "remove the second of the three original criteria" with no access to the original documentation or code, just the current code.

  • (nodebb)

    I prefer the original form (leaving aside the someBool == true bit).

    If random colleague from Business Requirements Department asks "so what happens if the part isn't in stock?", the original code lets you answer quickly. The modified code does not.

    And, as others have pointed out, this probably isn't even a performance concern; modern compilers are smart. Write for humans (including Future You).

  • nasch (unregistered)

    If random colleague from Business Requirements Department asks "so what happens if the part isn't in stock?", the original code lets you answer quickly. The modified code does not.

    Sure it does. The answer is, "it doesn't matter if it's in stock or not."

  • David Mårtensson (unregistered)

    I like the last note ;)

    If you can use TDWTF as your documentation, you really do have problems :P

Leave a comment on “Modus Pwned”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article