Is it bad practice to use accessors from within a class? [duplicate]

Question

This question already has answers here:

Accessing members in your own class: use (auto)properties or not? (5 answers)

Closed 8 years ago.

So I have a simple class, User, which is something like this (ignore horrible whitespace use, wanted to keep it brief to read online):

public class User
{
  private string username;
  public string Username 
  {
    get
    {
      return username;
    }set{
      if(Validate.usernameIsValid(value)){username = value;}
      else{throw new InvalidArgumentException("Username is invalid");}
    }
  }
  //some more fields

  public User(String argUsername)
  {
    this.Username = argUsername;
    //OR validate again before:
    username = argUsername;
  }

}

Is it better to use the public accessor within the class to use its validation? Or is that bad practice and in that case, should I re-validate before setting the private username field?

Answer 1

I'd recommend using the public setter over local setting of the variable, simply because there'll be one place - the setter - where all the logic related to validation is handled. But this is only effective if you follow this convention every where within the class and all the derived versions strictly.

One cannot assume that a member variable is not manipulated elsewhere in the class ( or its derived versions, if it is protected). Imagine another programmer debugging an error related to username validation. It can be a pleasant surprise to find out upon search, that all validations take place via the setter - so she doesn't haven't to debug multiple validation logic.

Answer 2

The answer depends on where the data is coming from. Since the driving point behind using a setter is to avoid duplicating your validation code, you have two basic cases:

• When the data is coming from outside your class (i.e. through a method parameter, as in your example) then you should call the setter, which would perform validation for you.
• When the data has been "sanitized" at the point where you are ready to assign, go straight to the variable, bypassing the validation.

The former case extends to situations where you read user input.

The later case includes situations where you restore object's state from a snapshot, or generate object's state internally. For example, if your setter has null/empty string validation, and your method wants to set the string to a string representation of a GUID, it's OK to skip the setter.

Answer 3

In most cases I use the public property because what is done there usualy needs to be done all the time. But there are exceptions (for example if validations needs to be done once). So you can't really can't tell in general.

Answer 4

Using public accessors within a class is OK, as well as using any public method. (After all, properties are just syntax sugar for getter/setter methods.)

Using accessors internally might be preferred if accessing a property from within a class has the same semantics as accessing it from outside.

In addition, accessors bring a consistent way to deal with regular and auto-implemented properties. (Auto-implemented ones just do not expose backing fields explicitly.)

Also, outside of constructors, almost for sure, accessors should be used for virtual properties.

Direct use of backing field is needed rather for back-door property manipulations.

But there is no general rule which approach to choose.

Answer 5

actually you have done well, self encapsulate fields make you set the value of property based on your needs easily: for example take a look at this:

private readonly ProductManufacturer _productManufacturer = new  ProductManufacturer();
private readonly IList<ProductCategory> _productCategories = new List<ProductCategory>();


public ProductManufacturer ProductManufacturer
{
    get { return _productManufacturer; }
}

public IEnumerable<ProductCategory> ProductCategory
{
    get { return _productCategories; }
}

now you can get the list of product category list else where, or you can only do some other operation on product category like this:

 public void AddProductCategory(ProductCategory productCategory)
        {
            _productCategories.Add(productCategory);
        }

for more information about self encapsulate fields take a look at this:

http://www.refactoring.com/catalog/selfEncapsulateField.html