views:

200

answers:

3

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

+10  A: 

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

naikus
+1, and I think it's weird to have add/remove something in an ImmutableXXX... it seems to be a non sense...
Sylvain M
yeah agreed.. my mistake..thanks
Shekhar
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?
Shekhar
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.
PartlyCloudy
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.
Donal Fellows
@Shekhar no, it does not, the way i put was wrong. Thanks for pointing out.
naikus
@Shekhar: I think in general you should stay away from the vanilla collections altogether, otherwise the bookkeeping required to copy mutable collections into a new mutable collection will become overwhelmingly error prone and a performance killer. You should look into immutable collections instead, of which you can find for Scala and I'm sure there are Java libraries out there too.
Juliet
+2  A: 

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.

Tom Hawtin - tackline
unmodifiableList will have no effect while the wrapper only protects write access to the wrappter, the underlying list can be changed every time
Tobias P.
@Tobias I don't follow. Did you miss the construction of the `ArrayList`?
Tom Hawtin - tackline
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
@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.
Tom Hawtin - tackline
thanks for your answer
Shekhar
A: 

"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

Mike