6

Findbugs reports a lot of EI_EXPOSE_REP and EI_EXPOSE_REP2 bugs on my code, each time I write getters and setters like this:

  public Date getDate() {
    return date;
  }
  public void setDate(final Date date) {
    this.date = date;
  }

I understand the meaning of the report, I should not expose the internal references of my objet to the outside world so that they can not be modified by a malicious/erronous code. A fix would be:

  public Date getDate() {
    return date == null ? null : date.clone();
  }
  public void setDate(Date date) {
    this.date = date == null ? null : date.clone();
  }

My question is not here. I am surprised that this report concerns ALWAYS Date. Why not all other mutable objects? I think this report goes for all mutable objects too, doesn't it ?

Should I extend this "good practice" to all my accessors that deal with mutable objects?

Give me your advice, thanks

moudug
  • 209
  • 1
  • 3
  • 13

2 Answers2

3

I would certainly expect this report to relate to all mutable objects, but I suspect that FindBugs is aware of certain common offenders.

I'm normally careful with exposing internal state via getters e.g.

public ArrayList<Trade> getTrades() {
   return trades;
}

means

  1. a client may be exposed to changes in your trade list
  2. a client may change that list that you passed out in good faith

As such there are two approaches.

  1. pass an immutable variant of that object (i.e. an object that can't be changed). In the above scenario you would take a read-only copy of that list and pass that out (you might argue that you could just take a simple read-write copy and pass that since it wouldn't affect the originating object, but that's counterintuitive)
  2. don't pass the object (list of trades) out, but rather have the owning object perform operations on that collection for you. This is perhaps the essence of OO - telling objects to do things for you rather than asking them for info and doing it yourself.

Similar arguments relate to setters and constructor arguments.

Note that you can find yourself copying lots of objects upon exposure in order to protect yourself, and potentially doing a lot of extra work. It's a technique to be used judiciously and it's worth understanding who your client objects are, and if you can control and/or trust them.

Brian Agnew
  • 268,207
  • 37
  • 334
  • 440
  • Hi Brian, thanks for your answer. What exactly do you call "**an _immutable_ variant of that object**" ? – moudug Nov 20 '12 at 13:35
  • Amended answer above. An immutable object is an object that can't be changed. e.g. no setters, fields final etc. – Brian Agnew Nov 20 '12 at 13:53
1

Date object has setMonth and other setters to manipulate the value where as most of the other mutables do not have setter to change its value (example Integer does not have any setter).

    Case 1 :

    Date date =  obj.getDate();        
    date.setHours(10);

    Case 2 :

    Integer i = obj.getI();
    i = 10;

Finbug considers only case 1 as a security threat.

StasM
  • 10,593
  • 6
  • 56
  • 103
Bala
  • 4,427
  • 6
  • 26
  • 29