Passing objects to Method and change attributes

Question

If I have a class which looks like the following:

class A {
   private List<B> bs = new ArrayList<B>;

   public List<B> getBs() {
    return bs;
   }
}

It holds a list of object of type B.

Another class makes use of this list and hands one Object of the list to yet another object of class C:

class SomeOtherClass {

   private C c;

   public void someMethod() {
   //code...
   ArrayList<B> bs = mA.getBs();
   c.setB(bs.get(0));

   }

}

Finally, some values of member fields of object b are changed.

class C {
    private B b;

    public void setB(B b) {
       this.b = b;
       this.b.ChangeValue();
    }
}

This way of course the objects in the original list gets modified, which is what I want. However, I find it extremely confusing that class C modifies the values of the object in list bs of class A. For another developer it would be hard to see that this method change the values. So I think what I do here is considered to be bad practice, isn't it?` What's a better way of writing such kind of code?

Answer 1

I would agree with your reasoning. This method:

public void setB(B b) {
    this.b = b;
    this.b.ChangeValue();
}

is not very nicely implemented. It should preferably be refactored. If that's not an option, it should clearly document that the B argument is modified when passed to the method. If possible, this should be made clear from the method name as well. Perhaps you could name it something like setAndUpdate(B b).

---

This method:

class A {
    private List<B> bs = new ArrayList<B>;

    public List<B> getBs() {
        return bs;
    }
}

may be acceptable:

• If the interface of A should allow insertion and deletion of B objects, I'd say it's acceptable to hand out the backing list to leverage the list-manipulation methods such as addAll, clear etc. provided by List.

• If bs is indeed part of the internal state of A, and A is concerned with modification of bs (such as if it need to maintain some invariant that involves bs) then it should not leak the backing data structure.

You'll find examples of both variants in the standard collections API. keySet and entrySet for instance expose collections that affect the map they were retrieved from. List.toArray on the other hand returns a copy.

If you want to expose a read only version of the backing list, you could do

// Return a read only view of bs
public List<B> getBs() {
    return Collections.unmodifiableList(bs);
}

Answer 2

I think what I do here is considered to be bad practice, isn't it?

Yes, exposing mutable portions of your class through a getter may be misleading, because the data is not encapsulated well enough. There is nothing wrong with changing the data, it's just that it is done in a way that does not immediately stand out as modification.

There is no universal rule to fix this. Generally, you should avoid returning mutable objects inside your class directly. Instead, provide getters and setters where appropriate, and do not give List<B> to your caller:

class A {
     private List<B> bs = new ArrayList<B>;

     public void getSizeB() {
         return bs.size();
     }
     public B getB(int i) {
         return bs.get(i);
     }
     public void setB(int i, B b) {
         bs.set(i, b);
     }
     public void addB(B b) {
         bs.add(B);
     }
     ... // Add more methods as needed
}

The idea is to stay in control of what gets into your List<B>: since there is no direct access, you can validate every B that gets into the list. The code that uses your class A must be explicit about all modifications that it makes, because it cannot get the list directly and do whatever it wants with it.

Answer 3

First of all, I'd say getBs() is a bad idea, since the caller of that method can clear the list or add elements to it, which would be a much more serious change to the state of A than just changing a property in one of the elements of the list.

You can replace it with getB(int), which returns the i'th element of that list. Now, if B is immutable, your job is done, and the caller of getB(int) can't change anything in the state of A.

If B is mutable, you can decide that getB(int) would return a copy of bs.get(i).

To summarize, I'd replace :

public void getBs() {
    return bs;
}

with either

public void getB(int i) {
    return bs.get(i);
}

or

public void getB(int i) {
    return new B(bs.get(i));
}

Of course, you should also add range checks, to make sure i is a valid index.