4

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;
    }



}

6 Answers 6

8

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.

10
  • 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. - to do this first I'd have to create such an interface, solely for the getters of Person class. And I'd need to do that for any other class like this, so it's a lot of code for a small improvement. Jun 4, 2016 at 12:17
  • 2
    A small but important improvement, I think Jun 4, 2016 at 12:28
  • @user4205580: well, it's less code than making lots of pass-through getters. And making a clone also takes some code, and it's also more work at runtime. Jun 4, 2016 at 12:39
  • 2
    Okay, right. I agree, returning a clone can be misleading - clients of my class might think they can modify the person of B by calling foo1 on it, but what they do is modify a clone, not the original object - which could cause problems. Jun 4, 2016 at 12:42
  • from where comes the idea that an interface is "a lot of extra code"? it's like 5 lines of code. I hear people say this all the time, but I never understood it
    – sara
    Jun 21, 2016 at 10:57
6

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.

34
  • 1
    @user4205580 When in doubt imagine how the system would work if it was a real system. You don't ask for car for the engine so you can then manually start it. You ask the car to start the engine, and you don't care how it does it. No client of the car needs to know how the engine start mechanism works, and has no need to getEngine from the Car object. Think about what each object's behaviour is, not what data it holds. Forget about data, data is largely irrelevant. One key design goal of Object Orientated design was to eliminate data completely. Jun 8, 2016 at 15:03
  • 1
    If for some reason the behaviour of the object results in a list it would. But a getter method is not behaviour. A car mechanic analogy doesn't really work because in an OO system the car would simply repair itself. If you are taking data out of objects to work on that data some where else in the system you have a flaw in your design and the first question to ask is is my data in the right object? The easiest way to avoid this is to forget about data completely until you have designed the behaviour of your objects. Then just define the data each object needs to carry out that behaviour. Jun 9, 2016 at 8:09
  • 1
    The logic should be placed in the schedule class, a schedule should know itself if it is valid or not if it is its responsible to tell the system it is valid. But that logic should be encapsulated in its own object, (eg. a ScheduleValidator) that the schedule can use to determine if it is valid or not. That validator can be passed into the schedule either at initialisation (ie dependency injection) or when the schedule is asked to validate itself. Basically the system says "Hey schedule, validate yourself with this validation rule object" or something like that Jun 9, 2016 at 12:35
  • 1
    I would go as far as saying, that the schedule should refuse any state change which would make it invalid. Nevertheless a) the schedule should validate b) would make use of the strategy pattern to swap out different rulesets. Jun 9, 2016 at 13:32
  • 2
    @user4205580 Generally if you are using a getter there is something wrong with your design, the data is in the wrong place or you are not thinking of as the object as a unit of behaviour. For example there is no reason you can't pass an object into the Job object and allow that object to carry out some action with that object which is then passed on. I don't know exactly what validation system you have in mind, but you should always tell an object to do something (possibly using another object) rather than ask for data out of the object. Jun 10, 2016 at 21:57
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)

2

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.

2

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

3
  • Just to clarify, is it completely wrong to create classes in OO language like Java, if their sole purpose is to hold data, i.e. using classes as data structures? IMO - not really. I've seen many cases of using a Person class, that just hold information about a given person (and contains getters, of course). How much more mess in the code would there be without Person class in such cases. Jun 21, 2016 at 10:35
  • 1
    Well nothing is "completely wrong", but it is a design smell, a reason to pause and think. It means you have an object with no behaviour, that exists simply to hold data. The first question would be if the behaviour is some where else why is the data not some where else as well. You don't have to have a Person class just because you have, say, a person record in a database. If your design is being lead by your data rather than by the behaviour of the system then you will end up with a hard to manage system because you will have data leaking between objects. Jun 21, 2016 at 10:41
  • 1
    Also just because developers do it doesn't mean it is a good idea. Software developers make bad software all the time. So seeing something that is common is not the same thing as seeing something that is good. There are a number of good books that get to the root of object orientated design (behaviour lead development) that I'll append to the answer above. Jun 21, 2016 at 10:43
0

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
    }
}

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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