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

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.