3

In order to reflect changes in your data to the UI you have to implement INotifyPropertyChanged, okay. If I look at examples, articles, tutorials etc most of the time the setters look like something in that manner:

public string MyProperty
{
  //get [...]
  set
  {
    if (_correspondingField == value)
    {
      return;
    }
    _correspondingField = value;
    OnPropertyChanged("MyProperty");
  }
}

No problem so far, only raise the event if you have to, cool. But you could rewrite this code to this:

public string MyProperty
{
  //get [...]
  set
  {
    if (_correspondingField != value)
    {
      _correspondingField = value;
      OnPropertyChanged("MyProperty");
    }
  }
}

It should do the same (?), you only have one place of return, it is less code, it is less boring code and it is more to the point ("only act if necessary" vs "if not necessary do nothing, the other way round act").

So if the second version has its pros compared to the first one, why I see this style rarely? I don't consider myself being smarter than those people that write frameworks, published articles etc, therefore the second version has to have drawbacks. Or is it wrong? Or do I think too much?

Thanks in advance

yveso
  • 103
  • 6
  • 1
    *"Or do I think too much?"* yes probably, but that's a common feature of a good engineer ;) – MattDavey Jun 21 '11 at 08:28
  • Egads. This has *nothing* to do with any of your tags. Also, google the "arrowhead antipattern". –  Jun 21 '11 at 13:14
  • @Will: Maybe not directly, but if you use the techniques from the tags you may come to a point where you ask yourself this question. At least it was my journey to it, maybe it can help someone else. What would be better tags (as I want to learn)? But of course you are completely right (especially as a moderator): First rule in IT: Don't be too friendly to a new customer, he could come back... Thanks for the google term anyway – yveso Jun 21 '11 at 18:38

4 Answers4

3

I find the second example more readable personally. I think one of the reasons the first example has become widespread is that Resharper will prompt to use this style (and automatically do the conversion), with the rationale that it "reduces nesting". Which is correct, but in simple cases like these I think readability is more important.

It comes down to a fundamental difference of opinion - there are those programmers out there who think that there should only ever be one "return" - one single exit point at the end of the method. Then there are those who think that there should always be an "early exit" if at all possible, which can lead to multiple "returns" throughout the method. Which do you prefer? :)

MattDavey
  • 8,897
  • 3
  • 31
  • 54
  • 1
    +1. I'm in the one return point camp. Multiple return points allows for some serious buggage. Excessive nesting should be solved by factoring out some extra functions. Early return should only be resorted to when there is evidence that not doing so is harming performance in a signifcant way. One reasonable exception is where functions only purpose is to loop and find an item, early exit is very useful and as long as the axiom "A function should do only one thing and do it well" is held to, its quite readable. – AnthonyWJones Jun 21 '11 at 12:21
  • Thanks for your answer. Didn't know that Resharper thing. Which I prefer? What about "it depends"? ;) – yveso Jun 21 '11 at 18:54
  • @AnthonyWJones I completely agree - often having multiple exit points is a sign that a method has multiple responsibilities and needs refactoring anyway. I think the only legitimate early exit strategies are to break out of a loop (as you mentioned), or on parameter validation where a return value might be more appropriate than an exception. – MattDavey Jun 22 '11 at 07:41
  • @Ivovic there's no right or wrong answer to the question, it's just something to think about :) – MattDavey Jun 22 '11 at 07:42
  • sure, there are of course situations where the early-exit/less-nesting way makes sense (and where I use it). My question was regarding my viewmodels, where the code is that simple like in the sample above. Prominent examples (Mr. Josh Smith's MSDN-MVVM article, code snippets in MVVM Light etc) take the first version, so is there something I don't see or is it (more or less) just a matter of taste. Thanks again and have a nice day. – yveso Jun 22 '11 at 10:04
1

I see the second style much more often than the first... but anyway it doesn't matter, the behavior is exactly the same. And no, I don't think there is any drawback with the second approach. It's just a matter of personal preference, choose the one you find most readable.

Thomas Levesque
  • 286,951
  • 70
  • 623
  • 758
0

Same as Thomas Levesque. I use myself the second one in my code, and I have seen it quite often. Both approach should perform the same.

Eilistraee
  • 8,220
  • 1
  • 27
  • 30
0

It reduces nesting.

In your example, its pretty clear, and its just a matter of taste, but it is much clearer when used in consecutive manner, as you can see here:

Invert "if" statement to reduce nesting

Usually I don't care if I get an event when the value is the same, so I leave that part out:

public string MyProperty
{
  get { return _correspondingField; }
  set { _correspondingField = value; OnPropertyChanged("MyProperty"); }
}
Community
  • 1
  • 1
Arcturus
  • 26,677
  • 10
  • 92
  • 107
  • 1
    It's not a question of you not caring. If I had to use your view models and its raising changed notifications when nothing has actually changed, it would confuse the f out of me. You should think about writing as if you're going to ship your classes to the public - unless you write alone and have no ambition to work with anyone else, in which case that's cool. Happy coding, bro. – Luke Puplett Jun 21 '11 at 08:30
  • 1
    Seriously, lighten up 'bro'. When you have dependency properties at your UI, they have a build in mechanism to deal with this stuff. Also its not like it will break expected behavior, since its the same value. My solution is easier on the eye, and for that once in a lifetime occurrence that the value is the same, its not going to slow down your UI, so why not leave that part out. You might want to check the part where someone is setting that same value twice, and fix that instead.. so YAGNI! – Arcturus Jun 21 '11 at 08:48
  • 1
    Appreciate the YAGNI approach, but it's called INotifyPropertyChanged, not INotifyPropertyMightBeChangedButDontCountOnIt ;) – MattDavey Jun 21 '11 at 13:24