Is this class fully Immutable?

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() {
        retu开发者_JS百科rn 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

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 :)

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.

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

2. 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.

3. 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.

4. 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).

5. 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