14

In my Java application I have some copy-constructors like this

public MyClass(MyClass src) {
    this.field1 = src.field1;
    this.field2 = src.field2;
    this.field3 = src.field3;
...
}

Now Netbeans 6.9 warns about this and I wonder what is wrong with this code?

My concerns:

  • Using the getters might introduce unwanted side-effects. The new object might no longer be considered a copy of the original.
  • If it is recommended using the getters, wouldn't it be more consistent if one would use setters for the new instance as well?

EDIT: The actual warning is "Access of private field of another object" and the only available action Netbeans offers is adding a @SuppressWarnings("AccessingNonPublicFieldOfAnotherObject")

My code is actually as trivial as the given example.

Daniel Rikowski
  • 71,375
  • 57
  • 251
  • 329
  • What is the warning from Netbeans? – Timothy Mar 12 '10 at 09:29
  • 1
    I can only speculate on the reason: Some people think that `private` means that only the current *object* can access the field (while in reality other objects of the same class can also access the field). Maybe this warning is meant as a reminder that this is actually possible and to prevent people from doing it accidentally. – Joachim Sauer Mar 12 '10 at 09:37

4 Answers4

8

I see no problem with the code. In fact, I share your concerns and would prefer to not use getters.

What exactly is the warning you get? Maybe it is related to something you are not showing us?

Update: Seems Netbeans is really complaining about just that. That is a rather controversial warning to ship with an IDE, I would think.

Thilo
  • 257,207
  • 101
  • 511
  • 656
  • 4
    Absolutely. The concept of private is for *private to the class implementation*, in my view. A copy constructor is part of the class implementation and can do what it likes with private data. – T.J. Crowder Mar 12 '10 at 09:31
  • Such a copy is a problem if the fields are not primitive, but fields of which the contents can change.Even if that's wanted you can still get concurrency issues. – extraneon Mar 12 '10 at 09:33
  • @extraneon: That seems to be outside of the scope of the warning. Wrapping things into getters is also not going to make a difference there. – Thilo Mar 12 '10 at 09:35
  • After doing some further research I agree. I'll disable this warning, as the only possible occurrences known to me (copy constructors and `equals`) are perfectly fine. – Daniel Rikowski Mar 12 '10 at 09:56
6

It reminds me of the discussion ADT vs. Object.

ADT can access the internal state of other instance of the ADT. The object philosophy would prevent that and enforce access through getter/setter to ensure representation independence.

It's been a deliberated choice that instances variable in Java are class-private, not object-private (In the later case only this.privateInstVar is allowed, not obj.privateInstVar).

This has both strength and weaknesses. It's espcially handy when it comes to cloning and equality. On the other hand it can be misused and break encapsulation.

It's almost a philosophical debate. But IMHO, what you are doing is fine.

ewernli
  • 38,045
  • 5
  • 92
  • 123
3

I don't know why NetBeans warns, but you are effectively making a shallow copy. Your copy shares field1, field2 and field3 with src and as such a modification of field3, say, a List, will reflect in the original.

private List field1;

public MyClass(MyClass src) {
    this.field1 = src.field1;
    this.field2 = src.field2;
    this.field3 = src.field3;

    field1.add(new Object()); // field1 of src also modified
...
}
extraneon
  • 23,575
  • 2
  • 47
  • 51
0

Perhaps the class could provide a shallow copy method which knows which data to return in a copied object. If needed, you could provide a deep-copy too.

public MyClass shallowCopy() {
  MyClass aCopy = new MyClass();
  aCopy.field1 = this.field1;
  aCopy.field2 = this.field2;
  aCopy.field3 = this.field3;
}
Timothy
  • 2,457
  • 19
  • 15