3

I've recently had an argument with a colleague about using getters (without setters) in a view-model classes used by XAML.

Example:

public string FullName
{
   get
   {
      return $"{FirstName} {LastName}";
   }
}
//call OnPropertyChanged("FullName") when setting FirstName or LastName

His argument was that you should only ever create get/set pairs which use a private field (with the setter raising the PropertyChanged event if the value has indeed been changed).

Example:

private string _fullName;
public string FullName
{
   get
   {
      return _fullName;
   }
   set
   {
      if (value != _fullName)
      {
         _fullName = value;
         OnPropertyChanged("FullName");
      }
   }
}
//Set FullName when setting FirstName or LastName

I, on the other hand, think it's fine to use getters (which calculate the output on the fly), provided they aren't updated all the time via too many PropertyChanged calls.

I don't think my colleagues approach is bad - I just think it's way more busy work which might simply be not needed. However he was sure my approach is bad and should be avoided at all cost.

As a counter example I pointed out that the MVVM base class from DevExpress has RaisePropertyChanged and RaisePropertiesChanged (for refreshing multiple properties) methods ready for use - both of which wouldn't be needed if all the properties were get/set pairs (the DevExpress base class also exposes a SetProperty method for use in setters, which also includes a PropertyChanged call). His argument was that "well, people are stubborn, keep doing it wrong, so DevExpress simply added those methods to make sure people are happy" which I found... odd, to say the least.

So what's the take here? Is it bad design to use getters in view-models designed to be used by XAML?

EDIT: I forgot to mention... my colleague also made the claim that using get/set pairs will always be faster. That does seem to be true as far as updating the UI is concerned, but he also used this as an argument that getters shouldn't ever be used. The whole argument started when I had a bit of performance issues when populating a data grid with more (i.e. 12k) rows where my VM used quite a few getters...

  • 3
    Aside from the fact that you can simplify your version to public string FullName => $"{FirstName} {LastName}";, you example is superior as it offers proper encapsulation. The use of setters should be avoided as much as possible. They are a definite code smell (ie an indication of likely bad design) – David Arno Feb 3 '16 at 12:22
  • @DavidArno Holy...! I had no idea I could write it in such a way! :D – Shaamaan Feb 3 '16 at 13:29
  • @DavidArno One more question - gbjbaanb mentions that only get/set pairs like the one my colleague pushes for are a bad smell... you suggest that all setters are a bad smell? In a hypothetical scenario where a model has FirstName and LastName properties (DB requirements) but the client wants to have an editable text box for FullName (bad idea, but hey, a client is a client)... it seems like a setter would be the best way to go about this, no? – Shaamaan Feb 3 '16 at 14:06
  • 1
    I fail to see how setters are a 'code smell' when working with WPF and MVVM. You need setters for two-way databinding with UI elements! – 17 of 26 Feb 4 '16 at 14:38
5

Your coworker is incorrect.

When working with UI logic in MVVM, it's common to have properties for binding that are derived from other data.

In these cases, you don't want to create another private member - that just adds a point of failure with no benefit.

Side note:

If you're using C# 6 then instead of hardcoded strings you can do

OnPropertyChanged(nameof(FullName));

This way, you'll get a compiler error if you change the name of the FullName property but forget to update corresponding calls to OnPropertyChanged.

  • 1
    Oooh, that nameof is also nice! That said, when NOT working with DevExpress I created an OnPropertyChanged(Expression) which could be invoked via OnPropertyChanged(() => PropertyName), which would do the exact same thing as nameof. DevExpress' base class has that built-in as well. – Shaamaan Feb 3 '16 at 14:19
  • 1
    Or just OnPropertyChanged(); if you declare void OnPropertyChanged([CallerMemberName] string propertyName) – Scroog1 Feb 3 '16 at 14:21
  • 1
    @Scroog1 Yes, if you're calling it from inside that same property's setter. But if you're calling OnPropertyChanged(nameof(FullName)); from the FirstName setter then it needs to be explicit. – 17 of 26 Feb 3 '16 at 14:23
  • 1
    @Shaamaan: If you use the OnPropertyChanged(() => PropertyName) you will have a run-time overhead (reflections) that I don't believe you will have when using nameof(), which should be evaluated at build-time, if I am not mistaken. – Robert Jørgensgaard Engdahl Feb 3 '16 at 19:56
  • @RobertJørgensgaardEngdahl Obviously OnPropertyChanged(() => Property) is the older solution and will be less optimal than the new one. :) But both solve the "name update" issue @17of26 mentioned. In any case, I've learned quite a bit from the answers to this question, which is awesome! :D – Shaamaan Feb 4 '16 at 9:03
3

Getters like your colleague expects, are a code smell. They're only there for language niggles anyway. What they're doing is exposing a private variable. To be honest, you might as well just access the private variable directly if that's all you're using getters for and cut out a layer of middleman.

Originally OO languages supported the use of accessor methods (eg getter and setters) as they intention was that they would perform validation and other logic on every get and set. It also hid the internal implementation details from the user of the class - so you could expose FullName, and then if you later changed the internals to store first and last names, you could update the getter without changing the clients who use your class. (and to expand further, when you extend your class to handle middle initial, that too can be done without having to update every caller)

Now over time, code generators like Devexpress and VS added shortcuts to help, by letting you declare a variable private and automatically type out the boilerplate to write the accessors. Unfortunately, some people think that because this happens, the boilerplated accessor is what you should use. Its nuts that people don't think for themselves and have descended into a cargo-cult attitude to coding.

There are articles on the web that describe how you should be designing classes, that say if all you're doing is exposing a variable as a getter, then you haven't actually designed anything.

And lastly, if you do store the same data twice (fullname as well as first and last names) then you are starting to get in a mess, as you will always have to do plenty of updating whenever any of the 3 change.

  • I forgot to mention in my question the matter of performance (I've edited the question now). Can you comment on possible UI performance issues when using get/set pairs vs getters? – Shaamaan Feb 3 '16 at 8:56
  • 1
    @Shaamaan that depends - if you're asking "should I cache results inside my getters" then that's a fair response.I tend to use a dirty flag to indicate if the data has changed and requires a call to OnPropertyChanged, but YMMV. – gbjbaanb Feb 3 '16 at 9:24
  • One more question - would your answer be the same if we were talking about private setters? – Shaamaan Feb 3 '16 at 14:08
  • @Shaamaan why would you use private setters? Update the variable directly, or if there's a lot of common logic involved then use a utility method. So I guess - it depends. – gbjbaanb Feb 3 '16 at 14:13
  • I don't WANT to use private setters. ;) I was thinking how a view-model looks like from the outside in the hypothetical "first, last and full-name" scenario. If the full name exposes a setter, then it's implied it can, in fact, be set by the view. A private setter would, at least from the outside, look more like the getter-only situation... I'm not saying this is in any way a good / bad way, just... thinking out loud. – Shaamaan Feb 3 '16 at 14:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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