Mutability and getters

Question

In the example below, I have a Person class and class B that holds a reference to a Person.

Person has a public foo1 method that can alter its state (it's a mutable object).

Suppose clients of class B want to know the salary, name and email of person object. Should I rewrite the getters from Person class in class B (without providing a getPerson method)? Or maybe getPerson is enough, as people will be able to call getSalary, getEmail and getName on it?

Knowing that Person is mutable, we should ask if we want clients of B to modify that person by calling foo1. We may decide to rewrite Person' getters in B or return a clone of that Person object, so that Person in class B won't be modified.

What about a simpler example, if Person was immutable (there would be no foo1 method in this case)? In B, should I have getPerson (which returns a clone, just in case Person will become mutable at a later point), or rewrite all Person's getters?

class Person{
    private String name;
    private int salary;
    private String email;
    //other private fields

    public void foo1(){
        //modify some fields here
    }

    public int getSalary(){
        return salary;
    }
    public String getName(){
        return name;
    }
    public String getEmail(){
        return email;
    }

}

class B{
    private Person person;

    public Person getPerson(){
        return person;
    }



}

Answer 1

Knowing that Person is mutable, we should ask if we want clients of B to modify that person by calling foo1. We may decide to rewrite Person' getters in B or return a clone of that Person object, so that Person in class B won't be modified.

If you do not want clients to modify the returned object, another option is to return an interface type which contains only the getters of Person.

What about a simpler example, if Person was immutable (there would be no foo1 method in this case)? In B, should I have getPerson (which returns a clone, just in case Person will become mutable at a later point), or rewrite all Person's getters?

No, you should have getPerson() and return a reference to the Person instance. Returning a clone "just in case Person will become mutable at a later point" is a ridiculous case of YAGNI, and adding lots of individual getters a horrible case of code duplication.

Don't overthink. Most of all, strive to make your code simple.

Answer 2

Your design of B is wrong when it leaks A.

This clearly violates the Law of Demeter and results in Train Wreck Code.

Your intention for composing an object is to extend its knowledge or behavior: Say you want to build a car, the car needs an engine. so it makes perfectly sense to equip the car with an engine.

As a driver, you want to use the API (so to say) of the car, i.e. not interfere with the engine but drive the car. The same goes for consuments of your class B. The fact, that B is composed of A should be unknown to its consuments.

If an external object needs to know something the A within B could answer, it should ask B.

Additionally: If you provide access to A, the result is tight coupling between the consuments of B and A, who rely on the implementation of A and B, which is not, what you want.

Answer 3

What is class B doing. Remember that OO is about telling objects to do things for you. So (for example) if B is a PersonnelManager, it could have a collection of Person objects and work at a higher abstraction level (e.g. giveRaise(percentage) ). I wouldn't expect it to proxy those Person objects for you (which is what you're implying in some of the above)

The exception to the above would be if B was some sort of DAO, but then I don't think your questions above apply at all)

Answer 4

For class B, is the fact that it has a reference to a Person just an implementation detail, or is it part of the spec of class B that it should provide access to a Person object?

That's the question, and the answer decides what interface class B should provide. Could it make sense to change from a reference to Person to a reference to some different class that might be more suitable? If you gave access to a Person, and suddenly you have no Person in class B anymore, you have lots of code changes to make. On the other hand, if that reference to Person is essential for users of class B, then make it available.

Answer 5

To expand on what I was saying in the comments, this is a simple example in Ruby of how you can do what @user4205580 is asking without having to use getter methods. For those unfamiliar with Ruby values starting with @ are private instance variables.

This is a quick and dirty example (writing it in a coffeeshop while waiting for my tea) and I don't claim it is awesome design, but rather a quick proof that in most cases where you think you need a getter a few minutes thinking about it can show that you don't.

class Job
  def initialize(name, start_time, end_time)
    @name = name
    @start_time = start_time
    @end_time = end_time
  end

  def starts_before(time)
    @start_time < time
  end

  def starts_after(time)
    @start_time > time
  end

  def conflict?(job)
    job.starts_after(@start_time) && job.starts_before(@end_time) # Test if job starts after we start, but before we end
  end
end


class Schedule
  def initialize
    @job_list = []
    @validators = []
  end

  def add_job(job)
    @job_list << job
  end

  def add_validator(validator)
    @validators << validator
  end

  def valid?
    @validators.all? do |validator|
      validator.run(@job_list)
    end
  end
end


class NoTimeConflictRule
  def run(jobs)
    # FYI product gets combo of every job eg [[1, 1], [1, 2], [1, 3], [2, 1]... etc
    # so we can compare every job to every other job. any? returns true if any
    jobs.product(jobs).each do |job_a, job_b|
      if (job_a != job_b) && (job_a.conflict?(job_b) || job_b.conflict?(job_a))
        return false # We have a time conflict, return false
      end
    end
    true # No conflict so return true for validation
  end
end


clean_shower = Job.new "clean shower", Time.utc(2016, 06, 12, 9, 45), Time.utc(2016, 06, 12, 10, 45)
walk_dog = Job.new "walk dog",  Time.utc(2016, 06, 12, 10), Time.utc(2016, 06, 12, 10, 30)
do_homework = Job.new "do homework", Time.utc(2016, 06, 12, 11), Time.utc(2016, 06, 12, 12)

invalid_schedule = Schedule.new
invalid_schedule.add_job clean_shower
invalid_schedule.add_job walk_dog
invalid_schedule.add_validator NoTimeConflictRule.new

puts "My invalid schedule is valid? - #{invalid_schedule.valid?}"

valid_schedule = Schedule.new
valid_schedule.add_job clean_shower
valid_schedule.add_job do_homework
valid_schedule.add_validator NoTimeConflictRule.new

puts "My valid schedule is valid? - #{valid_schedule.valid?}"

This code runs, and if you do you will get this output

$ruby stack_overflow.rb
My invalid schedule is valid? - false
My valid schedule is valid? - true

Update - some good books on object orientated design that focus on behaviour

Object Thinking

Designing Object Oriented Software

Business Engineering with Object Technology

Object Oriented Software Engineering

Smalltalk, Objects and Design

Answer 6

I will focus on the foo1() part of the question.

Person has a public foo1 method that can alter its state (it's a mutable object).

...

What about a simpler example, if Person was immutable (there would be no foo1 method in this case)?

I suppose your uncertainty about whether Person should be immutable reflects your uncertainty about whether foo1() should exist on Person. In other words, you have the choice of implementing it either way.

Here is an example of how to implement immutable Person while allowing a foo1() that seemingly has access to some mutable state. My insight is that the "some mutable state" does not need to be stored on the Person object.

// mutable and contains the additional mutable state
// used by foo1() but is not stored as part of Person.
class Foo
{
    // ...
}

class FooCollection
{
    private HashMap<Person, Foo> mapPersonToFoo; 
}

class Person
{
    public Person(string name, ..., FooCollection fooCollection)
    {
        this.fooCollection = fooCollection;
    }
    public void foo1()
    {
        Foo myFoo = fooCollection.getFoo(this);
        // use mutable foo, as long as it does not modify Person
    }
}