4

I am trying to convert a mutable class to an Immutable class following the advices given in Effective Java Item 15 (Minimize Mutability). Can anybody tell me whether the class I created is fully immutable or not?

Mutable Class

public class Record { public int sequenceNumber; public String id; public List<Field> fields; /** * Default Constructor */ public Record() { super(); } public Record addField(Field fieldToAdd) { fields.add(fieldToAdd); return this; } public Record removeField(Field fieldToRemove) { fields.remove(fieldToRemove); return this; } public int getSequenceNumber() { return sequenceNumber; } public String getId() { return id; } public List<Field> getFields() { return fields; } public void setSequenceNumber(int sequenceNumber) { this.sequenceNumber = sequenceNumber; } public void setFields(List<Field> fields) { this.fields = fields; } public void setId(String id) { this.id = id; } }

Field Class

public class Field { private String name; private String value; public Field(String name,String value) { this.name = name; this.value = value; } public String getName() { return name; } public String getValue() { return value; } }

Immutable Class

public class ImmutableRecord { private final int sequenceNumber; private final String id; private final List<Field> fields; private ImmutableRecord(int sequenceNumber, List<Field> fields) { this.sequenceNumber = sequenceNumber; this.fields = fields; this.id = UUID.randomUUID().toString(); } public static ImmutableRecord getInstance(int sequenceNumber, List<Field> fields) { return new ImmutableRecord(sequenceNumber, fields); } /********************* Only Accessor No Mutator *********************/ public int getSequenceNumber() { return sequenceNumber; } public String getId() { return id; } public List<Field> getFields() { return Collections.unmodifiableList(fields); } /********************* Instance Methods *********************/ public ImmutableRecord addField(Field fieldToAdd) { Field field = new Field(fieldToAdd.getName(), fieldToAdd.getValue()); List<Field> newFields = new ArrayList<Field>(fields); newFields.add(field); Collections.unmodifiableList(newFields); ImmutableRecord immutableRecord = new ImmutableRecord(sequenceNumber, newFields); return immutableRecord; } public ImmutableRecord removeField(Field fieldToRemove) { Field field = new Field(fieldToRemove.getName(), fieldToRemove.getValue()); List<Field> newFields = new ArrayList<Field>(fields); newFields.remove(field); Collections.unmodifiableList(newFields); ImmutableRecord immutableRecord = new ImmutableRecord(sequenceNumber, newFields); return immutableRecord; } }

Thanks

Shekhar

Improve this question

3 Answers 3

Reset to default
11

No its not, the list fields should be copied and not stored a direct reference

private ImmutableRecord(int sequenceNumber, List<Field> fields) { this.sequenceNumber = sequenceNumber; this.fields = fields; // breaks immutability!!! this.id = UUID.randomUUID().toString(); }

If some one modifies the List fields reference, your class will also reflect it. Better to copy the contents of the collection into another one before assigining to this.fields

Also your class is appears mutable since it has add and remove methods :)

Improve this answer
Sign up to request clarification or add additional context in comments.

6 Comments

+1, and I think it's weird to have add/remove something in an ImmutableXXX... it seems to be a non sense...
I have one question.. You said my class is mutable because it has add and remove method but I am returning a new instance of ImmutableRecord.I am not modifying the original ImmutableRecord. Are you saying just having these methods make it mutable?
The problem is that you can't control the fields that will be passed as a parameter to the getInstance() method. You're not able to prevent that someone will save the reference to this field outside of your class and modify it.
Depends on whether you consider an immutable reference to mutable data to be OK or not. That in turn depends on the nature of the underlying conceptual model you are working with.
@Shekhar no, it does not, the way i put was wrong. Thanks for pointing out.
|
2

As naikus points out, the fields argument in the constructor may be modified outside of the class. It might also be a "non-standard" List implementation. So,

 this.fields = fields;

should be changed to

 this.fields = new ArrayList<Field>(fields);

or possibly

 this.fields = Collections.unmodifiableList(new ArrayList<Field>(fields));

There are pros and cons of making the field an unmodifiable list. Primarily, it says what you mean. It prevents errors/maintenance engineers. The allocation is a bit hit and miss - you don't allocate on every get; allocations are optimised (potentially escape analysis coming along); having objects hanging around is not a good idea because it slows down the garbage collection (and to a much smaller extent consume memory).

Also make all classes and fields - say what you mean, and there are some subtleties.

"Add" methods are fine. Look at BigInteger, say (although ignore certain features of it!).

A slightly controversial point of view is that all that get in those accessor methods in an immutable class are noise. Remove the get.

Making the constructor private and adding a static creation method named of, adds a bit, but you almost never require a "new" object. Also allows type inference, before we get the diamond operator currently in JDK7. private constructors also allow removing copying mutables when creating the new instance in addField and removeField.

equals, hashCode and perhaps toString are nice to have. Although there is YAGNI vs construct interfaces (concept, not Java keyword) as an API.

Improve this answer

4 Comments

unmodifiableList will have no effect while the wrapper only protects write access to the wrappter, the underlying list can be changed every time
@Tobias I don't follow. Did you miss the construction of the ArrayList?
Can you please explain what you meant by "private constructors also allow removing copying mutables when creating the new instance in addField and removeField."? Thanks
@Shekhar Currently addField needs to construct an ArrayList and the constructor constructs another ArrayList. This can be reduced to a single ArrayList construction, so long as it is done through a private interface.
1

"1. Don't provide any methods that modify the object (known as mutators).

  1. Ensure that no methods may be overridden. This prevents careless or malicious subclasses from compromising the immutable behavior of the class. Preventing method overrides is generally done by making the class final.

  2. Make all fields final. This clearly expresses your intentions in a manner that is enforced by the system. Also, it may be necessary to ensure correct behavior if a reference to a newly created instance is passed from one thread to another without synchronization, depending on the results of ongoing efforts to rework the memory model.

  3. Make all fields private. This prevents clients from modifying fields directly. While it is technically permissible for immutable classes to have public final fields containing primitive values or references to immutable objects, it is not recommended because it precludes changing the internal representation in a later release (Item 12).

  4. Ensure exclusive access to any mutable components. If your class has any fields that refer to mutable objects, ensure that clients of the class cannot obtain references to these objects. Neither initialize such a field to a client-provided object reference nor return the object reference from an accessor. Make defensive copies (Item 24) in constructors, accessors, and readObject methods (Item 56)."

http://wiki.glassfish.java.net/attach/JavaProgramming/ej.html#immutablerecipe

Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.