Choosing a Base Descriptor

Question

This code is very large and seems quite confusing. I tried to refactor it to make it more readable and make the code seem less...(scroll below). However, I am not sure if this can be refactored more.

 TableTypeDescriptor baseDecriptor = base.CustomTypeDescriptor as TableTypeDescriptor;

        if (this.IsObjectBeingValidated)
            return baseDecriptor; 

        if (base.CustomTypeDescriptor != null)
        {
            if (baseDecriptor.OldTableDescriptor != null && this.IsNewInCorrection && !this.IsReadOnly)
            {
                return base.CustomTypeDescriptor;
            }
            else if (this.Locked)
            {
                return base.CustomTypeDescriptor;
            }
            else if (this.ImportedDataMD5 != null && !this.IsReadOnly)
            {
                return base.CustomTypeDescriptor;
            }
            else
            {
                if (baseDecriptor.OldTableDescriptor != null)
                {
                    return baseDecriptor.OldTableDescriptor;
                }
                else
                    return base.CustomTypeDescriptor;
            }
        }

        return base.CustomTypeDescriptor;

I then refactored it as follows. Is it possible to refactor it more?

TableTypeDescriptor baseDescriptor = base.CustomTypeDescriptor as TableTypeDescriptor;

                if (base.CustomTypeDescriptor != null || !this.IsObjectBeingValidated)
                {
                    if ( (!this.IsReadOnly && (this.IsNewInCorrection || this.ImportedDataMD5 != null))  ||
                         this.Locked                                
                       )
                        baseDescriptor = base.CustomTypeDescriptor;
                    else
                    {
                        if (baseDescriptor.OldTableDescriptor != null)
                            baseDescriptor = baseDecriptor.OldTableDescriptor;
                        else
                            baseDescriptor = base.CustomTypeDescriptor;
                    }
                }

                return baseDescriptor;

Answer 1

You should use braces {} for single if statements too. This makes your code less errorprone.

---

If you are refactoring some code you should be sure to have good tests at your hand to validate that the refactored code behaves like the old code.

---

By extracting the check for all these this.xx() properties to a separate method like

private static bool ShouldReturnCurrentDescriptor()
{
    return (this.IsObjectBeingValidated) ||
           (this.Locked) ||
           (!this.IsReadOnly && (this.IsNewInCorrection || this.ImportedDataMD5 != null));
}  

The former code can be refactored to

TableTypeDescriptor baseDecriptor = base.CustomTypeDescriptor as TableTypeDescriptor;

if (ShouldReturnCurrentDescriptor() ||
    (baseDecriptor.OldTableDescriptor == null))
{
    return baseDecriptor;
}
return baseDecriptor.OldTableDescriptor;  

Answer 2

Here are some things to make it shorter (NOTE: It makes the code shorter, but it's not always more readable):

1. You can use ? : for if-else constructions when you assign a value. For example:

if(i > 5)
{
    x = i;
}
else
{
    x = 0;
}

can be rewritten to:

x = (i > 5) ? i : 0;

2. You could return beforehand to reduce if-nesting. For example:

public void someMethod(int i){
    if(i > 5)
    {
        int x = i * 10;
        x += 5;
        someOtherMethod(x);
    }
}

can be rewritten to:

public void someMethod(int i){
    if(i <= 5) return;

    someOtherMethod((i * 10) + 5);
}

These are just some examples on how to shorten your code.

---

Now let's get back to your code:

I could shorten it to..:

TableTypeDescriptor baseDescriptor = base.CustomTypeDescriptor as TableTypeDescriptor;

if (base.CustomTypeDescriptor == null || this.IsObjectBeingValidated) return baseDescriptor;

baseDescriptor = ((!IsReadOnly && (IsNewInCorrection || ImportedDataMD5 != null)) || Locked)
    ? base.CustomTypeDescriptor;
    : ((baseDescriptor.OldTableDescriptor != null)
        ? baseDescriptor.OldTableDescriptor
        : base.CustomerTypeDescriptor);
return baseDescriptor;

This is a lot shorter as you can see, BUT it's definitely not more readable! Instead, you need to find a midway between short code and readability. Personally if I had your code, I would change it to something like this:

TableTypeDescriptor baseDecriptor = base.CustomTypeDescriptor as TableTypeDescriptor;
if(baseDescriptor == null || base.CustomerTypeDescriptor) return null;

if(IsObjectBeingValidated) return baseDescriptor;

if((baseDescriptor.OldTableDescriptor != null && IsNewInCorrection && IsReadOnly)
    || (ImportDataMD5 != null && IsReadOnly)
    || Locked)
{
    return base.CustomTypeDescriptor;
}

return baseDescriptor.OldTableDescriptor != null
    ? baseDescriptor.OldTableDescriptor
    : base.CustomTypeDescriptor;

Perhaps you could also separate your big if into a separate method. So instead of:

if((baseDescriptor.OldTableDescriptor != null && IsNewInCorrection && IsReadOnly)
    || (ImportDataMD5 != null && IsReadOnly)
    || Locked)
{
    return base.CustomTypeDescriptor;
}

you could make it:

if(checkSomething())
{
    return base.CustomTypeDescriptor;
}

...

// TODO: obviously change this name. I don't know what your application is about, so can't really think of a good name for it
private void checkSomething(){
    return (baseDescriptor.OldTableDescriptor != null && IsNewInCorrection && IsReadOnly)
        || (ImportDataMD5 != null && IsReadOnly)
        || Locked;
}

I personally don't find it too readable like this, but without your complete code I can't really refactor it sufficient enough.

All in all it's not always easy to make more readable code and it takes practice and knowledge. Also, there are some good refactor tools out there to make things a lot easier. I currently use ReSharper for my C# WPF project, and can recommend it.

Answer 3

Someone mentioned this above, but I don't feel that they gave it enough attention in their answer.

Your code doesn't follow C# standards and is hard to read

TableTypeDescriptor baseDescriptor = base.CustomTypeDescriptor as TableTypeDescriptor;

if (base.CustomTypeDescriptor != null || !this.IsObjectBeingValidated)
{
    if ( (!this.IsReadOnly && (this.IsNewInCorrection || this.ImportedDataMD5 != null))  ||
        this.Locked                                
    )
        baseDescriptor = base.CustomTypeDescriptor;
    else
    {
        if (baseDescriptor.OldTableDescriptor != null)
            baseDescriptor = baseDecriptor.OldTableDescriptor;
        else
            baseDescriptor = base.CustomTypeDescriptor;
    }
}

return baseDescriptor;

Always use brackets, you can write one lined if statements without them, but it doesn't make for readable code when the else statement that accompanies it uses brackets.

Whenever you have an if/then statement inside of an else statement you should look at possibly using an else if statement to reduce nesting.

I used the outside if statement as a short circuit to get out of the method by returning the baseDescriptor

At first I looked at what I had left and I said that there is only 1 instance where you want to return baseDescriptor.oldTableDescriptor which brought me to:

TableTypeDescriptor baseDescriptor = base.CustomTypeDescriptor as TableTypeDescriptor;

if (base.CustomTypeDescriptor == null || this.IsObjectBeingValidated)
{
    return baseDescriptor;
}

if (baseDescriptor.OldTableDescriptor != null)
{
    baseDescriptor = baseDecriptor.OldTableDescriptor;
}
else
{
    baseDescriptor = base.CustomTypeDescriptor;
}
return baseDescriptor

But then I thought about your confusing if statement and thought that you most probably had a really good reason for it, so I had to leave it alone, but I did change the whole structure to an if/else if/else statement with the returns inside the structure, like so:

TableTypeDescriptor baseDescriptor = base.CustomTypeDescriptor as TableTypeDescriptor;

if (base.CustomTypeDescriptor == null || this.IsObjectBeingValidated)
{
    return baseDescriptor;
}

if ((!this.IsReadOnly && (this.IsNewInCorrection || this.ImportedDataMD5 != null)) || this.Locked)
{
    return base.CustomTypeDescriptor;
}
else if (baseDescriptor.OldTableDescriptor != null)
{
    return baseDecriptor.OldTableDescriptor;
}
else
{
    return base.CustomTypeDescriptor;
}

I saw no reason to make the condition 2 lines on the initial if statement