Is assigning a value to a property through a set accessor via its container's get accessor a bad thing?

Question

Consider the following code:

class Program
{
    static void Main(string[] args)
    {
        Department deathStar = new Department { Name = "Death Star" };

        Console.WriteLine("The manager of {0} is {1}.", deathStar.Name, deathStar.Manager.FullName);

        deathStar.Manager.FirstName = "Lord";

        Console.WriteLine("The manager of {0} is {1}.", deathStar.Name, deathStar.Manager.FullName);

        Console.ReadLine();
    }
}

public class Person
{
    public string FirstName { get; set; }
    public string LastName { get; set; }

    public string FullName
    {
        get
        {
            return this.FirstName + " " + this.LastName;
        }
    }
}

public class Department
{
    public string Name { get; set; }

    public Person Manager { get; private set; }

    public Department()
    {
        this.Manager = new Person { FirstName = "Darth", LastName = "Vader" };
    }
}

which produces the following output:

The manager of Death Star is Darth Vader.
The manager of Death Star is Lord Vader.

Even though I can't change Manager to be a different or new instance of Person (private set accessor), I can change it's properties (which have public set accessors).

So, is assigning a value to a property through a set accessor via its container's get accessor a bad thing? In other words, is this a code smell?

EDIT:

This is just a sample to illustrate a point. Person and Department were created for this sample only.

Answer 1

Not necessarrily.

For example, look at the parameters collection on a SqlCommand object. You can change the items in the collection, but you can't assign a new parameters collection to the command object.

Take your example, if you had a UI maintaining Person objects, and you needed to change a persons first name, it's perfectly valid to change the persons first name, leave the last name and PersonId fields alone, then update a database table using the PersonId field.

It all sounds OK to me

Answer 2

I'd say that the line:

deathStar.Manager.FirstName = "Lord";

is perfectly clear in its intent, so it doesn't bother me.

What does bother me is the private set on Manager.

This means that once a department is created, it's manager can never be changed.

As a model for reality, this is a big time smell.

Answer 3

I think this is one of the reasons people tout functional languages so much - no "side effects". I don't think there is anything technically wrong with this, but it can make maintenance very difficult. To paraphrase Jurrasic Park, "...just because you can do something, doesn't mean you should!"

Answer 4

Is it "a bad thing"? Not intrinsically. Is it a sign that you should re-examine your design of the classes? Yes. Why is something that creates a Department modifying a Person object? Should you be implementing a ChangeManager method on Department instead? Maybe, maybe not. But it's a question worth asking.

Answer 5

Yes, this is a code smell.

While this seems like a fairly harmless practice, public properties expose the fields of your class to arbitrary change and create needless dependence on implementation details. You may as well be using a struct. (I'm not implying that structs are bad either. Procedural programming is just as useful as OOP. It all depends on the problem being solved) The point of encapsulation is to hide implementation details from the class' dependents. Rather you should expose abstract interfaces that allow dependent classes to modify the internal data without needing to know its implementation. Consider the following examples:

public interface Vehicle {
    double FuelTankCapacityInGallons{get;}
    double GallonsOfGasoline{get;}
}

public interface Vehicle {
    double getPercentFuelRemaining();
}

Both interfaces will get you the amount of remaining gasoline. The first through concrete terms, the second through abstraction. If the size of the tank changes or you need to implement a European car the first example would require changes to both the Vehicle and whatever class is using it. While the second would only require changes to the Vehicle implementation.

The example I used is from the book Clean Code - A Handbook of Agile Software Craftsmanship by Robert C. Martin.

I would also look up Law of Demeter.

Answer 6

It's not a problem as long as Person is not a struct : if it is, then the Manager property will return a copy of the Person, and you will be setting the FirstName property on that copy, not on the department's Manager. But anyway, the compiler would scream if you tried to do that...

Answer 7

It's a question of OOP purity.
Not all pieces of code should be pure OOP. It's a matter of project complexity, number of programmers involved, maintainability, etc.
Your example is very simple, and that's why you can read so many opinions
It is sometimes hard to illustrate the advantages of OOP with simple examples.

Anyway, my thoughts:
It smells.
The line of code that really bothers me is

deathStar.Manager.FirstName = "Lord";

It can easily change to something like

deathStar.Manager.Address.City.Name = "Paris"

So in one line of code you see 4 classes in use! This makes a very strong assumption that the coupling of all 4 classes remains the same. In other words, it keeps the classes highly coupled.
You can resolve this by working with interfaces rather than concrete classes, and you can adopt the "Tell don't ask" guideline (Yahoo search; yes, Yahoo).
Here's a nice PDF that discusses OOP and refactoring (in Java, but the ideas are understood easily), see "Transitive coupling" section.