Conditionally Limit Property Access

Is there a better way to limit access to the Occupation and Employer properties?

This class is simply designed to collect a person's (potential customer's) employment information. EmploymentStatus can be Employed, SelfEmployed, Unemployed, R开发者_开发问答etired, etc...

I only want users of this class to be able to set Employer and Occupation if the person is indeed employed.

public class EmploymentInformation
{
    private const string _EmploymentStatusNotEmployedMessage = "Employment status is not set to employed";

    private string _occupation;

    private Company _employer;

    /// <summary>The person's employment status<example>Employed</example></summary>
    public EmploymentStatus EmploymentStatus { get; set; }

    /// <summary>The person's occupation<example>Web Developer</example></summary>
    public string Occupation
    {
        get
        {
            if (IsEmployed)
            {
                return _occupation;
            }
            throw new ApplicationException(_EmploymentStatusNotEmployedMessage);
        }

        set
        {
            if (IsEmployed)
            {
                _occupation = value;
            }
            throw new ApplicationException(_EmploymentStatusNotEmployedMessage);
        }
    }

    /// <summary>The person's employer</summary>
    public Company Employer
    {
        get
        {
            if (IsEmployed)
            {
                return _employer;
            }
            throw new ApplicationException(_EmploymentStatusNotEmployedMessage);
        }

        set
        {
            if (IsEmployed)
            {
                _employer = value;
            }
            throw new ApplicationException(_EmploymentStatusNotEmployedMessage);
        }
    }

    private bool IsEmployed
    {
        get
        {
            return EmploymentStatus == EmploymentStatus.Employed 
                || EmploymentStatus == EmploymentStatus.SelfEmployed;
        }
    }

    /// <summary>
    /// Constructor for EmploymentInformation
    /// </summary>
    /// <param name="employmentStatus">The person's employment status</param>
    public EmploymentInformation(EmploymentStatus employmentStatus)
    {
        EmploymentStatus = employmentStatus;
    }
}

Anything wrong with simply returning null if the value is not set? That's fairly common practice. If Employer does not exist it's value is null. Why it's null may not be relevant. In addition force the employment status to be set within the ctor of the class itself.

Forcing developers to set properties in a particular order is a dangerous design: it makes the interface misleading and encourages mistakes.

Instead, consider making EmploymentInformation objects immutable:

// Constructor for retired / unemployed people
public class EmploymentInformation(EmploymentStatus status) {}

// Constructor for self-employed people - we know their status
public class EmploymentInformation(string occupation) {}

// Constructor for people employed by others - we know their status
public class EmploymentInformation(string occupation, Company employer) {}

public bool IsEmployed { get; }
public string Occupation { get; }
public Company Employer { get; }

Firstly, why is it possible to construct an object of EmploymentInformation if there is no Employer?

As far as possible, you should not allow an object to be constructed in an invalid state. You can express these constraints in the constructor of your object either using Guard Clauses or Code Contracts.

public class EmploymentInformation
{
    public EmoloymentInformation(Employer employerInstance)
    {
        if(employerInstance == null)
            throw new ArgumentNullException();
    }

Secondly, you can use the Null Object pattern so that you don't have to throw exceptions. Just create appropriate class for EmptyEmployer and return them as shown below.

        public Company Employer
            {
                get
                {
                    return IsEmployed ? _employer : Employer.Empty; 
// Employer.Empty is static property which return an instance of EmptyEmployer just like string.Empty.


    }

A new answer:

Given that the object is strictly to hold data regarding the users CURRENT employement status, it's still wrong.

As @Jeff Sternal said you shouldn't force dev's to assign parameters based on a particular order. In the event the object needs to be serialized/deserialized you could end up with a lot of errors.

Instead you should provide a validation function. Something like bool IsValid(); When that method is called perform the business logic validation to ensure that the object is in an acceptable state. You could have it simply return a false if not, throw an exception (please don't), or have it send a status code back as to why the object is not currently valid.

Typically you throw data into an object THEN you validate the object is good prior to persistence. The above is just one way of doing this. Others include having a Business Logic library which separates the logic completely from the data classes (personally, I never understood why you'd do this, but a lot of people swear by it.).

I've not had any experience in this, but somewhere I would consider looking is in Code Contracts.

Have a look at these links:

http://social.msdn.microsoft.com/Forums/en/codecontracts/thread/1ca2d371-4b85-479d-9e00-64c84e372f02

http://msdn.microsoft.com/en-us/devlabs/dd491992.aspx

You can decorate your properties with "requirements" suitable to your application. Looks cool to use, and it appears to be half integrated into the IDE.

This looks wrong from a logic perspective.

The object is called "EmploymentInformation" and has a property called "EmploymentStatus"

This seems to either allow for situations where the employmentinformation deals with active or term'd employees; or, to allow for employment history.

If either of those are true, then it seems to me that you can have an Occupation but have an EmploymentStatus of something like "NotEmployed" for whatever reason.

After all, let's see the record is initially created where the EmploymentStatus is employed. Then later the status is changed to "NotEmployed" The next time you go to load the object you are going to lose data.

If you were being strict you might argue that an unemployed person does not have an occupation or employer, so a person object should not have these properties. That leads to something like this.

class Person
{
    public EmploymentStatus EmploymentStatus { get; set; }
}

class EmployedPerson : Person
{
    public string Occupation { get; set; }
    public Company Employer { get; set; }
}

However in practice this unforgiving object model will be cumbersome to work with as you will need to know whether or not a person is employed before you can instantiate an object. It will also be difficult to change between being employed and unemployed as you will have to create a new object and copy everything across.

The clinical distinction isn't worth it. I think it's just as correct and in fact more logical to ask an unemployed person who their employer is and for them to reply with "I haven't got one" rather than be unable to ask the question in the first place.

For me, this would be a more flexible person class.

class Person
{
    public Person()
    {
        this.EmploymentStatus = EmploymentStatus.Unemployed;
    }

    public void Hire(Company employer, string occupation)
    {
        this.Occupation = occupation;
        this.Employer = employer;
        this.EmploymentStatus = EmploymentStatus.Employed;
    }

    public void Fire()
    {
        this.Occupation = null;
        this.Employer = null;
        this.EmploymentStatus = EmploymentStatus.Unemployed;
    }

    public EmploymentStatus EmploymentStatus { get; private set; }
    public string Occupation { get; private set; }
    public Company Employer { get; private set; }
}