Spaghetti return statement

Question

I've just came upon this fine return statement :) I am curious how would you rewrite this, I will post my edited version later.

public bool Changed(bool Cascading)
    {
        return (myValue.Id.IsImport ||  
            isChanged || 
                ( 
                    ((!IsNull) && 
                        (
                            (!myValue.Equals(initialValue)) || ((Cascading ? 
                                                                            (((IBLObject)myValue).Changed()) : 
                                                                            (((IBLObject)myValue).Id.Changed())))
                         )
                    ) 
                    || (isNull != wasNull)
                )
            );
    }

Regards.

P.S. this was a single line return :)

Answer 1

Class properties should use PascalNotation. Arguments should use camelNotation.

Ideally method names include verbs, so their purpose is clear.

My edit focuses on readability.
I broke the code down into clear and simple parts. I managed to remove all those extra parenthesis that were making it hard and confusing to read.

    // TODO: Verb might need fixing. Take care not to conflict with class-level property.
    public bool HasChanged(bool cascading) {
        if (myValue.Id.IsImport
            || isChanged
            ) {
            return true;
        }
        if (!IsNull) {
            var myVal = (IBLObject)MyValue;
            bool hasChanged = cascading
                ? myVal.HasChanged()
                : myVal.Id.HasChanged();
            if (hasChanged || !MyValue.Equals(InitialValue)) {
                return true;
            }
        }
        return IsNull != WasNull;
    }

Answer 2

@ANeves: I hate when people nit pick about camel/pascal/hungarian/etc notations. The only general rule that matters is - "Your code should match the style of the project. The project should have one concise style."

Heres what I would use:

public bool changed(bool cascading)
{
    if (myValue.Id.IsImport || isChanged) return true;
    if (isNull) return isNull != wasNull;
    if (!myValue.Equals(initialValue)) return true;
    return cascading ? ((IBLObject)myValue).Changed() : ((IBLObject)myValue).Id.Changed();
}

I feel this is the best answer because it clearly breaks out the boolean logic he had above without changing the thought process behind it. It simply refactors his checks into easy to follow lines.

@Jesse Slicer: I prefer this method to the one you suggested because it has obvious performance benefits. All the additional 'if' statements in yours will result in a mess of JMPs that will slow down execution. I also feel this is much easier to read and understand with a passing glance.

Answer 3

I think I've unwound that Gordian Knot of Boolean Logic there and came up with this:

    public bool Changed(bool cascading)
    {
        if (this.myValue.Id.IsImport)
        {
            return true;
        }

        if (this.isChanged)
        {
            return true;
        }

        if (!this.IsNull)
        {
            if (!this.myValue.Equals(this.initialValue))
            {
                return true;
            }

            if (cascading)
            {
                if (((IBLObject)this.myValue).Changed())
                {
                    return true;
                }
            }
            else
            {
                if (((IBLObject)this.myValue).Id.Changed())
                {
                    return true;
                }
            }
        }

        if (this.isNull != this.wasNull)
        {
            return true;
        }

        return false;
    }

Answer 4

Building on ANeves answer, I wonder if the cascading behavior should instead be pulled into IBLObject, since you only ever look at data from the IBLObject to determine the result.

// TODO: HasChanged? Verb might need fixing.
public bool IsChanged(bool cascading) {
    if (myValue.Id.IsImport
        || isChanged
        ) {
        return true;
    }
    if (!IsNull) {
        if ((!MyValue.Equals(InitialValue)) || ((IBLObject)MyValue).IsChanged(cascading)) {
            return true;
        }
    }
    return IsNull != WasNull;
}

The IBLObject.IsChanged method could then be changed to something similar to the following:

public bool IsChanged (bool cascading) {
   if (!cascading)
      return Id.IsChanged ();

   // original IsChanged code here
}