iScreen = iScreen < 8 ? iScream : ...

David MacDermot

The fellow who wrote this (shall remain nameless :) ) several years ago was enamored of the ternary operator (hmm... Lispy). It seemed to him rather clever at the time; that was, until he encountered it again during maintenance. The variable name he chose turned out to be somewhat prophetic in a phonetic sort of way.

        byte\[\] arySend = FetchCommand(cmd.CHANGE\_THE\_ACTIVE\_DISPLAY);

        int iScreen = this.cboDisplayName.SelectedIndex;

        iScreen = iScreen < 8 ? iScreen + 1 :
         (7 < iScreen && iScreen < 10 ? iScreen + 4 :
          (10 == iScreen ? iScreen + 5 :
           (11 == iScreen ? iScreen + 10 :
            (11 < iScreen && iScreen < 16 ? iScreen + 12 :
             (15 < iScreen && iScreen < 20 ? iScreen + 24 :
              (19 < iScreen && iScreen < 22 ? iScreen + 45 :
               (22 == iScreen ? iScreen + 53 :
                (23 == iScreen ? iScreen + 57 :
                 (23 < iScreen && iScreen < 26 ? iScreen + 65 :
                  (25 < iScreen && iScreen < 28 ? iScreen + 72 :
                   (27 < iScreen && iScreen < 30 ? iScreen + 152 :
                    (30 == iScreen ? iScreen + 160 :
                     (30 < iScreen && iScreen < 33 ? iScreen + 169 :
                      (33 == iScreen ? iScreen + 170 :
                       (34 == iScreen ? iScreen + 171 :
                        (35 == iScreen ? iScreen + 175 :
                         (35 < iScreen && iScreen < 41 ? iScreen + 176 :
                          (41 == iScreen ? iScreen + 203 :
                           (42 == iScreen ? iScreen + 208 :
                            iScreen = iScreen + 212 ))))))))))))))))))); // 43 == iScreen

        arySend\[2\] = Convert.ToByte(iScreen);
        arySend\[3\] = 0x01; // Refresh screen

Paul Conrad

Nice :laugh:

"Any sort of work in VB6 is bound to provide several WTF moments." - Christian Graus

egenis

iscream too now!

Andrei Straut

David MacDermot wrote:

enamored of the ternary operator

I love the ternary operator too, but this...this...is just too much. Please poke my eyes out now!

Full-fledged Java/.NET lover, full-fledged PHP hater. Full-fledged Google/Microsoft lover, full-fledged Apple hater. Full-fledged Skype lover, full-fledged YM hater.

BobJanova

This is actually a fairly elegant construct if done correctly – a value-returning if-else stack, switch/case, or in this case quantiser into ranges. He's obfuscated it with the parens and indentation, and the unnecessary lower bound checks. If written as

iScreen = iScreen < 8 ? iScreen + 1 :
iScreen < 10 ? iScreen + 4 :
iScreen < 11 ? iScreen + 5 :
iScreen < 12 ? iScreen + 10 :
iScreen < 16 ? iScreen + 12 :
iScreen < 20 ? iScreen + 24 :
iScreen < 22 ? iScreen + 45 :
22 == iScreen ? iScreen + 53 :
23 == iScreen ? iScreen + 57 :
iScreen < 26 ? iScreen + 65 :
iScreen < 28 ? iScreen + 72 :
iScreen < 30 ? iScreen + 152 :
30 == iScreen ? iScreen + 160 :
iScreen < 33 ? iScreen + 169 :
33 == iScreen ? iScreen + 170 :
34 == iScreen ? iScreen + 171 :
35 == iScreen ? iScreen + 175 :
iScreen < 41 ? iScreen + 176 :
41 == iScreen ? iScreen + 203 :
42 == iScreen ? iScreen + 208 :
iScreen + 212;

... or, in fact, since the iScreen+ is in all of them, like

iScreen = iScreen +
iScreen < 8 ? 1 :
iScreen < 10 ? 4 :
iScreen < 11 ? 5 :
iScreen < 12 ? 10 :
iScreen < 16 ? 12 :
iScreen < 20 ? 24 :
iScreen < 22 ? 45 :
22== iScreen ? 53 :
23== iScreen ? 57 :
iScreen < 26 ? 65 :
iScreen < 28 ? 72 :
iScreen < 30 ? 152 :
30 == iScreen? 160 :
iScreen < 33 ? 169 :
33 == iScreen? 170 :
34 == iScreen? 171 :
35 == iScreen? 175 :
iScreen < 41 ? 176 :
41 == iScreen? 203 :
42 == iScreen? 208 :
212;

... it's not too bad. It's still not the right way to code this, which would be a map of upper bound to delta

OrderedMap<int, int> screenDeltas = { 7=>1, 9=>4, 10=>5, ..., 42=>208, 43=>212 };

function getScreenDelta(iScreen){
foreach(MapEntry<int, int> delta in screenDeltas)
if(iScreen <= delta.Key) return delta.Value;
}

Adriaan Davel

I don't see the problem, just look at the comments...

____________________________________________________________ Be brave little warrior, be VERY brave

Jonathan C Dickinson

It can be done 'better' (I think I have done up to 4 with that style before). I would likely land up using a switch for all the == cases, and have the < cases in the default case (using the ternary operator style you laid out). Otherwise, I don't really like the ordered map. Another way to do it would be to:

int[] screenDeltas = new[] { 1, 1, 1, 1, 1, 1, 1, 1, 1, 4, 4, /* ... */ };
/* ... */
iScreen += iScreen < 0 ? 1 :
iScreen > 42 ? 212 : /* Is 42 significant here? Are you building Deep Thought? */
screenDeltas[iScreen];

(Clearly code would be used to generate that array initializer) Because array lookups are O(1) and I love my premature optimization.

He who asks a question is a fool for five minutes. He who does not ask a question remains a fool forever. [Chineese Proverb] Jonathan C Dickinson (C# Software Engineer)

Reese Currie

The code quality is reminiscent of the legacy code I used to maintain that was written in the 70's and early 80's, before Ed Yourdon et. al. started talking about structured programming, which is itself a very old paradigm. I count a cyclomatic complexity of 51 (academia says 10 is "maintainable", really 18 or so if you stretch it) and it's full of "magic numbers" that have no obvious meaning other than they appear to be "screen numbers". It is kind of depressing to see that kind of code being written in a modern language, 30 years after the first efforts were made to educate people out of these kinds of practices.

agolddog

Wouldn't it be easier to bind the values of each item in the checkbox to the value this thing would produce and then just take the SelectedValue?

BobJanova

Yes, or at least bind the items to an object, of which this magic number was a property.

RafagaX

First, send my congratulations to your fellow, because he have a superior understanding of the use of the ternary operator, being smart (and brave) enough to nest that number of levels (the max i can nest is 4 whitout being lost in the logic). Second, slap him in the face, because this is a nightmare to maintain and a shame to see in any program, given that even a humble switch-case would have been better than this mess. :-)

CEO at: - Rafaga Systems - Para Facturas - Modern Components for the moment...

SeattleC

I believe Yourden wrote about structured programming in the late 1960's, and it was well known to professionals then. Your legacy code writers had no excuse. (Like they ever do...) There must be a selection-bias thing at work here wherein well-written legacy code doesn't need to be maintained, because it's well-written, and the legacy code that needs to be maintained, needs maintenance because it is bad code. The other possibility is intolerable; that there are some kids in middle-school today who will be cursing our beautiful code in 20 years. That would mean that even great coders aren't great every day. Just because you're Mozart doesn't mean people will remember ALL your music. Lots of it was produced under deadline pressure, and was only ok.

David MacDermot

BobJanova wrote:

Yes, or at least bind the items to an object, of which this magic number was a property.

That is exactly what I did when I had the pleasure of encountering this code while creating the next generation of the application. I employed a java enum style class in C#.

public class Display
{
public static readonly Display OpeningSplashdisplay =
new Display(1, "Opening Splash display");
public static readonly Display StartUpDateConfig =
new Display(2, "start up date config");

//
// Skip a lot of Displays
//

/// /// Enumerate the Display objects.
/// 
public static IEnumerable Values
{
    get
    {
        object temp = null;
        FieldInfo\[\] fields = typeof(Display).GetFields(); // Obtain all fields
        foreach (FieldInfo field in fields)
        {
            temp = field.GetValue(null);
            if (temp is Display)
                yield return (Display)temp;
        }
    }
}

private readonly byte value;
private readonly string name;

/// /// Enum constructor.
/// 
/// The numeric value of this object.
/// The string representation of this object.
Display(byte value, string name)
{
    this.value = value;
    this.name = name;
}

/// /// Get the numeric value of this object.
/// 
/// The value as byte.
public byte ToByte() { return value; }

/// /// Get the string representation of this object.
/// 
/// A string.
public override string ToString() { return name; }

/// /// Convert byte representation to Language object.
/// 
/// A byte.
/// A Display enumerated object.
public static Display ParseByte(byte value)
{
    foreach (Display d in Display.Values)
    {
        if (d.ToByte() == value)
            return d;
    }

    // If we reach here return default
    return Display.OpeningSplashdisplay;
}

}

Here I load the combobox from the data structure.

cmbDisplay.Items.AddRange(new List(Display.Values).To

Brisingr Aerowing

:eek:

Bill Gates is a very rich man today... and do you want to know why? The answer is one word: versions. Dave Barry Read more at [BrainyQuote](http://www.brainyquote.com/quotes/topics topic_technology.html#yAfSEbrfumitrteO.99)[^]