Is this class fully Immutable?

Question

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

Answer 1

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