Neatest/most idiomatic way to rewrite this If in C#

Question

I have this if-else statement which does what I want. What it's doing is pretty straightforward as you should be able to tell.

if (width != null && height != null)
{
    if (top != null && left != null)
    {
        ret.type = VMLDimensionType.full;
    }
    else
    {
        ret.type = VMLDimensionType.size;
    }
}
else
{
    if (top != null && left != null)
    {
        ret.type = VMLDimensionType.positon;
    }
    else
    {
        ret.type = VMLDimensionType.unset;
    }
}

The enum being referred to is:

private enum VMLDimensionType
{
    unset = 0,
    full = 1,
    size = 2,
    position = 3
}

It's so straightforward I'm sure there's a much more terse and more readable way to express this.

NB If it wasn't for the ridiculous 'one-brace per line' rule that VS imposes by default I probably wouldn't be so bothered. Eg in VB I could lose about 10 lines from this code block! (any thoughts on that as an aside?)

If I didn't miscount it, the VB.NET version would be 7 lines shorter, a version without braces 12 lines. — Bobby, Jan 05 '11 at 11:55
Can you post the `VMLDimensionType` enum? It might be simpler if it were a Flags enum. — Ani, Jan 05 '11 at 11:58
@El: well, just change the defaults. VS luckily allows this. Having each brace on an extra line like this really *is* a waste and doesn’t contribute to readability (IMHO). — Konrad Rudolph, Jan 05 '11 at 12:52

score 9 · Answer 1 · answered Jan 05 '11 at 11:58

9

bool hasPosition = (top != null && left != null);
bool hasSize = (width != null && height != null);

if (hasSize)
{
    ret.type = hasPosition ? VMLDimensionType.full : VMLDimensionType.size;
}
else
{
    ret.type = hasPosition ? VMLDimensionType.positon : VMLDimensionType.unset;
}

answered Jan 05 '11 at 11:58

Tim Lloyd

37,954
10
100
130

Coding with least number of lines isnt the best IMHO. Code with less number of lines with a clear readability makes it the best. This one fits the bill. – Danish Khan Jan 05 '11 at 12:04
+1 I like it, nice and readable actually. Kind of like reading from a chart. Similar to others but still.. – El Ronnoco Jan 05 '11 at 12:07
Apologies, I've changed my accepted answer to Ani's as I like the way they've made the enum reflect the logic of what is going on. – El Ronnoco Jan 05 '11 at 12:10

score 8 · Accepted Answer · answered Jan 05 '11 at 12:05

8

One option would be to make VMLDimensionType a Flags enumeration:

[Flags]
enum VMLDimensionType
{
    Unset = 0,
    Size = 1,
    Position = 1 << 1,
    Full = Size | Position
}

And then:

ret.Type = VMLDimensionType.Unset;

if(width != null && height != null)
    ret.Type |= VMLDimensionType.Size;

if (top != null && left != null)
    ret.Type |= VMLDimensionType.Position;

answered Jan 05 '11 at 12:05

Ani

111,048
26
262
307

Hmm, I may have to change my accepted answer :) This makes more sense as the enum is reflecting the actual logic... – El Ronnoco Jan 05 '11 at 12:09
1

@cibacity Full is implicit when Size and Position have both been set. That's the beauty of it. Doing the logical `OR` makes this happen. – El Ronnoco Jan 05 '11 at 12:14
@El Ronnoco: Dammit I need some sleep - of course it does, I'm being dense - doh! – Tim Lloyd Jan 05 '11 at 12:14
Why do you write `1 << 1` and not simply `2`? – Oliver Jan 05 '11 at 12:22
1

@Oliver: You're right, I could do that. This is how I normally write flags enums. To me, it clarifies that we are setting the second bit (1 shifted left by a bit). – Ani Jan 05 '11 at 12:25
@Ani: Shouldn't you then be consequent and also write `1 << 0` instead of `1` ;-) – Oliver Jan 05 '11 at 12:33
@Oliver: I suppose, but to me, that's the "base" case. – Ani Jan 05 '11 at 12:34

score 5 · Answer 3 · answered Jan 05 '11 at 11:57

5

How about this:

bool hasSize = width != null && height != null;
bool hasPosition = top != null && left != null;

if (hasSize && hasPosition)
{
    ret.type = VMLDimensionType.full;
}
else if (hasSize && !hasPosition)
{
    ret.type = VMLDimensionType.size;
}
else if (!hasSize && hasPosition)
{
    ret.type = VMLDimensionType.positon;
}
else
{
    ret.type = VMLDimensionType.unset;
}

answered Jan 05 '11 at 11:57

Mark Byers

811,555
193
1,581
1,452

+1 I quite like the readability that this solution gives. It's still quite big, but that may just be me getting used to all these braces :) – El Ronnoco Jan 05 '11 at 12:02
@El Ronnoco On single line statements you CAN remove the braces - I would recommend keeping them though, as it makes merging changes in source control easier, and gives you a definitive groupings that your eye can run over. – Jan 05 '11 at 12:09
@Will yes, when I first started with C# I used to leave out the braces for single line code blocks. Then I had to make some of them two line code-blocks.... – El Ronnoco Jan 05 '11 at 12:16

score 2 · Answer 4 · answered Jan 05 '11 at 12:10

2

I would like to extract GetDimensionType() method. And make it not so small, but more readable and self-descriptive.

private VMLDimensionType GetDimensionType()
{
    bool hasSize = width != null && height != null;
    bool hasPosition = top != null && left != null;

    if (hasSize && hasPosition)
        return VMLDimensionType.full;

    if (hasSize)
        return VMLDimensionType.size;

    if (hasPosition)
        return VMLDimensionType.positon;

    return VMLDimensionType.unset;
}

Usage:

ret.type = GetDimensionType();

answered Jan 05 '11 at 12:10

Sergey Berezovskiy

232,247
41
429
459

Where are `top,left,width` and `height` coming from? Or are they being accessed as members? – El Ronnoco Jan 05 '11 at 12:18
Yes, I suppose this is a members of some object. Anyway, if they are not, you can pass them as parameters. – Sergey Berezovskiy Jan 05 '11 at 12:22
+1 for mid-function returns. Note how nicely they turn the `else if`s of all the other solutions into simpler `if`s. – Carl Manaster Jan 05 '11 at 13:27

score 1 · Answer 5 · answered Jan 05 '11 at 11:59

1

What about this:

if(width != null && height != null)
    ret.type = top != null && left != null ? VMLDimensionType.full : VMLDimensionType.size;
else
    ret.type = top != null && left != null ? VMLDimensionType.positon : VMLDimensionType.unset;

answered Jan 05 '11 at 11:59

TJHeuvel

12,403
4
37
46

Didnt see Chibacity's comment before i posted :( – TJHeuvel Jan 05 '11 at 12:00
1

Yes, but not the neatest. His is better imo – TJHeuvel Jan 05 '11 at 12:04
@1 Ahh a nice bit of humility! – El Ronnoco Jan 05 '11 at 12:15

Neatest/most idiomatic way to rewrite this If in C#

5 Answers5