Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

share|improve this question
1  
Just because you can does not mean you should. –  Loki Astari Mar 28 '12 at 15:10
1  
Given this "if" statement, thedev should. –  Randy Mar 29 '12 at 12:20
    
@thedev what is the point of the isChanged variable if it isn't at least controlling the current level's change status? –  Randy Mar 29 '12 at 12:32
    
@thedev Also, what is the intended difference between the current level's ischanged and myValue changed()? –  Randy Mar 29 '12 at 12:47

4 Answers 4

up vote 6 down vote accepted

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;
    }
share|improve this answer
    
clearly better ... but I still see room for improvement :) I prefer to put every curly bracket on a separate line, seems clearer to me. –  thedev Mar 28 '12 at 15:27
1  
I did too, but now I prefer concise over clear. My only indecision when refactoring was whether to have return IsNull != WasNull; or if(IsNull != WasNull) { return true; } return false; like @JesseCSlicer did. –  ANeves Mar 28 '12 at 15:51
1  
In your refactor, I would recommend changing the method name to something else because it conflicts with a class level variable/property. –  Randy Mar 29 '12 at 12:28
    
This is a great job. My 2 cents would be to store MyValue in a local variable so you could do away with the two casts and 8 parenthesees ((IBLObjuect)MyValue)... –  Dan-o Apr 4 '12 at 19:41
    
@Boo Good suggestion, thanks; included. (Also the one by @Randy.) –  ANeves Apr 4 '12 at 19:59

@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.

share|improve this answer
2  
I 100% disagree with "obvious performance benefits". The optimizing compiler and the JITer will be doing its best to make the optimal run-time code and the amount of time saved will be negligible compared to any readability and maintainability gains. Even better would be if there were come honest-to-goodness commenting within this method. –  Jesse C. Slicer Mar 28 '12 at 20:31
    
umm, I'm not so sure this is easier to read to be honest than Jesse C.Slicers implementation. Compactness doesn't always make things easier to read. –  dreza Mar 28 '12 at 20:36
    
Jesse your right that the performance would be minor and most likely negligible but I was always taught to focus on performance even when its only micro seconds. It all adds up. –  Chris Frazier Mar 28 '12 at 20:39
    
And dreza, compactness would be using the original post on a single line. My idea is no were near as compact as it could be. Programmers need not fear single line statements. Embrace it and things become easier. Why use three lines for a simple if statement?? –  Chris Frazier Mar 28 '12 at 20:40
    
On the nit picking: you are right; but I still feel that language-standards are important. (Notation is one [standard], position of brackets is not one.) I would prefer to "waste" lines with then-statements that to have it harder to read due to uneven indentation of the then-statements. Also, I advise trying to first avoid the trap of premature optimization and then to try to use a profiler to find the bottlenecks. –  ANeves Mar 29 '12 at 15:55

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;
    }
share|improve this answer
    
even though it's fairly longer it seems much clearer, and I think the execution path can be determined faster when debugging this –  thedev Mar 28 '12 at 15:29
    
-1: this code is different! It isNull=true; wasNull=false this returns false, and should return true. –  ANeves Mar 28 '12 at 15:55
    
Should be fixed. –  Jesse C. Slicer Mar 28 '12 at 16:05
    
Seems to be fixed. –  ANeves Mar 29 '12 at 15:41

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
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.