CODEWITHSUNDEEP
c#refactoringcode-snippetscode-readabilityconditional-statements
El Ronnoco
El Ronnoco

Reputation: 11922

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

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

Upvotes: 2

Views: 228

Answers (5)

Sergey Berezovskiy
Sergey Berezovskiy

Reputation: 236238

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();

Upvotes: 2

Ani
Ani

Reputation: 113412

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;

Upvotes: 8

TJHeuvel
TJHeuvel

Reputation: 12608

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;

Upvotes: 1

Tim Lloyd
Tim Lloyd

Reputation: 38444

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;
}

Upvotes: 9

Mark Byers
Mark Byers

Reputation: 838326

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;
}

Upvotes: 5

Related Questions