• The Ternary King (unregistered)

    comment.frist = frist ? comments.Length == 0 : frist;

  • (nodebb)

    Surely DocumentStatusConsts.New is TRWTF here. DocumentStatusConsts??? That speaks of many ills in the codebase, and a ternary in place of an else-less if is neither here nor there compared to such a thing.

  • The Ternary King (unregistered)

    Seriously though, this isn't that WTFy. It's presumably disabling things that shouldn't be allowed except on a brand new document - there might be a design WTF in that concept, but that's not a code problem. Yes, you could write it with &= or a nested if instead, but is that actually any clearer?

  • (nodebb)

    That said, the worst ternary I ever saw was also a replacement for a else-less if, but far scarier, and can be paraphrased as:

    somecondition ? somevariable = someexpression : 0;

    That was the entirety of a statement. It was not an element in a larger expression.

  • (nodebb) in reply to The Ternary King

    Yes, this is much clearer:

    if ( item.Enabled )
    item.Enabled = (Document.Status == DocumentStatusConsts.New);

    It expresses what is going on: we want to update item.Enabled if it is true.

  • Jaloopa (unregistered) in reply to Steve_The_Cynic

    somecondition ? somevariable = someexpression : 0;

    I wrote something along those lines exactly once, while learning C++ in my first week of lessons. I was immediately told never to do something so silly again

  • Duct_Tape_Coder (unregistered)

    I'd put the condition in the loop

    foreach (var item in documentMenuItem.DropDownItems.Where(i => i.Enabled)) item.Enabled = Document.Status == DocumentStatusConsts.New;

  • Not Not Logical (unregistered)

    All of the ternary/if logic should be removed and the line simplified to the following:

    item.Enabled = !(!item.Enabled || Document.Status != DocumentStatusConsts.New)

  • ooOOooGa (unregistered) in reply to Steve_The_Cynic

    OK. Explain that one a bit more please.

    I'm hoping that you aren't having problems with using constants in general. That is much preferred over scattering literals around.

    So something wrong with tracking a document's status?

  • Naomi (unregistered) in reply to Steve_The_Cynic

    Not necessarily - it could be a clumsily-named enumeration. DocumentStatus.New doesn't sound so WTF-y, does it?

  • Naomi (unregistered) in reply to ooOOooGa

    On the other hand, it could also be a pile of unrelated constants, or not even an enum at all, which is what I think Steve's getting at. That would be a WTF.

  • (nodebb) in reply to ooOOooGa

    Names for interesting values ("constants") are a good thing, if they are done correctly(1). What's suggestive of bad things in the codebase, though, is the "DocumentStatusConsts" name. Why is it not an enum? What else is in there that isn't an actual status value, meaning that it wouldn't be in the enum? (Example: substatus codes where a particular status has two subcases that are close enough that we consider them identical, but their representations - in another field - are defined in DocumentStatusConsts)

    (1) See the endless vituperation surrounding e.g.

    #define ZERO 0
#define ONE 1

    or, worse:

    #define ZERO 1
  • SyntaxError (unregistered) in reply to Steve_The_Cynic

    Grossssss!

  • maybee (unregistered)

    This could "make sense" if item.Enabled is a property with attached setter, having side-effects. Still wtf, though.

  • (nodebb)

    Another possibility comes to mind--this used to make sense, then something changed.

  • Conrad Buck (unregistered)

    One thing I notice is that developers have been told they should never use a control structure without {}. This is ostensibly because of an SSL bug which apparently was caused by this code:

    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
    goto fail;
    goto fail;

    I think after the game of universal best practices telephone, this has resulted in the idea that ternaries are always safer than single line if expressions. I catch myself doing it sometimes. But if you work in a modern codebase with a code formatter it would be a lot harder to sneak that mistake through code review. Even without a code formatter that duplicated line error couldn't have easily occured if the entire expression was a single line.

  • JJ (unregistered) in reply to Steve_The_Cynic

    I don't see the problem with #define ZERO 1. That's something you do because you're the kind of bastard who enjoys dividing by zero, blowing raspberries at mathematicians.

  • James (unregistered)

    If you're going to be snarky you should at least provide a solution that work to the specs.

  • EmbeddedSpree (unregistered) in reply to The Ternary King

    Rewriting it with &= changes the behaviour, as that doesn't do short-circuit evaluation so that Document.Status will be evaluated even if !item.Enabled.

    But since presumably Document.Status == DocumentStatusConsts.New is loop-invariant, why not hoist it out of the loop anyway?

  • (nodebb)

    All I remember is that in Algol 60 you actually had to write "if", "then", and "else". and you could do it in an assignment statement. With longer keywords (not ':' and '?') it became tedious and you didn't do it (even though you "could")!!

  • RLB (unregistered)

    Ahem.

    if (Document.Status != DocumentStatusConsts.New) { item.enabled = false; }

  • SFAN (unregistered)

    item.enabled &= Document.Status != DocumentStatusConsts.New

  • Chris (unregistered)

    I think once again someone's been thinking that a one-liner ternary has better performance than an if-statement written over a couple of lines (4 if you include new lines for braces).

  • (nodebb)

    Not sure if this is a WTF, even though it is bad code.

    But your interpretation of what it does isn't right: If the thing is enabled, set it to disabled (or enabled) depending on this other feature. Otherwise leave it disabled.

  • (nodebb) in reply to SFAN

    item.enabled &= Document.Status != DocumentStatusConsts.New

    That's a bitwise & and also tests for the inverse of what you want.

  • (nodebb) in reply to Not Not Logical

    Or apply De Morgan's law to get

    item.enabled = !!(!!item.enabled && !(Document.Status != DocumentStatusConsts.New)

  • (nodebb) in reply to Not Not Logical

    Or apply De Morgan's law to get

    item.enabled = !!(!!item.enabled && !(Document.Status != DocumentStatusConsts.New)

    Addendum 2020-06-11 07:20: )

Leave a comment on “A Tern Off”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article