2
public class ImmutabilityOfReferenceInstance {

    public static void main(String[] args) {

         MClass mc = new MClass();

         mc.setId(1);
         ImClass imc1 = new ImClass(mc);
         System.out.println("imc1 = "+imc1);

         mc.setId(2);
         ImClass imc2 = new ImClass(mc);
         System.out.println("imc2 = "+imc2);         

    }
}

final class ImClass {

    final private MClass mClass;

    public ImClass(MClass mClass) {
        this.mClass = mClass;
    }

    public MClass getmClass() {
        return mClass;
    }   

    @Override
    public String toString() {      
        return String.valueOf(mClass.getId());
    }

}


class MClass {
    private int id;

    public int getId() {
        return id;
    }
    public void setId(int id) {
        this.id = id;
    }   
}

I want to provide complete immutablity to the class IMClass, As we can see IMclass is immutable but it has an instance variable mclass that is the reference of MClass and MClass is a mutable class. I have tried changing the getter method getmClass() as below

public MClass getmClass() {
        return (MClass) mClass.clone();
    }

but it is not allowing me to do so, Could some one please correct it that where i am getting wrong. Thanks in advance

I have tried this but still getting the same result, values are getting updated
public class ImmutabilityOfReferenceInstance {

    public static void main(String[] args) {

         MClass mc = new MClass();

         mc.setId(1);
         ImClass imc1 = new ImClass(mc);
         System.out.println("imc1 = "+imc1);

         mc.setId(2);
         ImClass imc2 = new ImClass(mc);
         System.out.println("imc2 = "+imc2);         
    }
}

final class ImClass {

    final private MClass mClass;

    public ImClass(MClass mClass) {
        this.mClass = (MClass)mClass.clone();
    }

    public MClass getmClass() {
        return (MClass)mClass.clone();
    }   

    @Override
    public String toString() {      
        return String.valueOf(mClass.getId());
    }

}

class MClass implements Cloneable{
    private int id;


    public int getId() {
        return id;
    }
    public void setId(int id) {
        this.id = id;
    }   

    @Override
    public Object clone() {
        try {
            return super.clone();
        } catch (CloneNotSupportedException e) {
            throw new RuntimeException(e);
        }
    }
}
Noorus Khan
  • 1,342
  • 3
  • 15
  • 33
  • 2
    Please define "it is not allowing me to do so". – Marvin Jul 30 '15 at 12:10
  • 5
    Also note that whoever constructs the instance of `ImClass` already has a reference to the mutable original anyway, so it's still not fully immutable - you'd need to clone in the constructor. – Jon Skeet Jul 30 '15 at 12:11
  • 3
    You haven't edited the "but it is not allowing me to do so" to explain what you mean. – Jon Skeet Jul 30 '15 at 12:12
  • that i want to know exactly where to change my code – Noorus Khan Jul 30 '15 at 12:13
  • 1
    What we mean is, when you say "it is not allowing me to do so" do you mean you get a compilation error? Runtime exception? Incorrect results? We still don't know what is happening. – takendarkk Jul 30 '15 at 12:15
  • The method clone() from the type Object is not visible – Noorus Khan Jul 30 '15 at 12:28
  • 1
    This sounds like an [XY problem](http://meta.stackexchange.com/questions/66377/what-is-the-xy-problem). Could the OP give an actual example instead of `ImClass` / `MClass`? The intuitive solution is to remove the reference to the mutable class from the immutable one entirely. – Mick Mnemonic Jul 30 '15 at 12:32
  • "I have tried this but still getting the same result, values are getting updated" - of course the value of `MClass.id` changes. I'm not sure if we already fully understood your problem. Can you please share a few more words about what behaviour you'd expect from your code? – Marvin Jul 30 '15 at 13:55

6 Answers6

1

There are a lot of good ideas floating around. Here is how I would summarize it:

  • Avoid using clone if possible, and favor using a copy-constructor instead. See Joshua Bloch's thoughts on this matter.
  • To ensure immutability, you need to make sure you copy the MClass instance that is passed to the ImClass constructor. Otherwise, whoever originally passed the MClass instance can still make changes to it.
  • Consider creating an immutable wrapper around the MClass class, perhaps by using inheritance.

This is one way that this can be achieved. There are certainly other ways:

public class ImmutabilityOfReferenceInstance {

    public static void main(String[] args) {

        MClass mc = new MClass();

        mc.setId(1);
        ImClass imc1 = new ImClass(mc);
        System.out.println("imc1 before = " + imc1);

        mc.setId(2);
        System.out.println("imc1 after = " + imc1); // continues printing 1.

        imc1.getmClass().setId(3); // changes not allowed on the immutable copy, throws exception.
    }
}

public final class ImClass {

    final private MClass mClass;

    public ImClass(MClass mClass) {
        this.mClass = (mClass == null ? null : mClass.createImmutableCopy());
    }

    public MClass getmClass() {
        return mClass;
    }

    @Override
    public String toString() {
        return String.valueOf(mClass.getId());
   }
}

public class MClass {

    private int id;

    public int getId() {
        return id;
    }

    public void setId(int id) {
        this.id = id;
    }

    public MClass createImmutableCopy() {
        return new ImmutableMClass(this);
    }

    private static class ImmutableMClass extends MClass {

        public ImmutableMClass(MClass src) {
            super.setId(src.getId());
        }

        @Override
        public void setId(int id) {
            throw new UnsupportedOperationException("immutable instance.");
        }
    }
}

EDIT: How to make the clone method work

If you still want to do it the cloning way, make sure you follow these 2 steps:

  1. Expose the clone as a public method (as already suggested), but, ideally, without swallowing the exception so that you don't get an inexplicable NullPointerException if something doesn't work. Although, technically, the CloneNotSupportedException exception should never happen if you don't forget step #2.

Like this:

@Override
public Object clone() {
    try {
        return super.clone();
    } catch (CloneNotSupportedException e) {
        throw new RuntimeException(e);
    }
}
  1. Make sure that the MClass implements the Cloneable interface.

Like this:

public class MClass implements Cloneable {
    // ...
}

But again, to make sure that the private MClass instance within the ImClass is "immutable", you'll need to call clone in 2 places:

  • In the ImClass.getmClass() method, as you are already doing.
  • Also in the ImClass constructor. If you forget this one, then it is still possible to modify it, so immutability hasn't been achieved fully.

Like this:

public ImClass(MClass mClass) {
    this.mClass = (MClass)mClass.clone();
}

EDIT 2: About why it appears that your code is still not working

The code should be working now, but if I look at your current main method, you are not testing immutability correctly. You are checking the values from 2 different instances of ImClass.

The following is a more valid test:

public static void main(String[] args) {

     MClass mc = new MClass();
     mc.setId(1);

     ImClass imc = new ImClass(mc);
     System.out.println("imc = " + imc); // should print 1

     mc.setId(2);

     System.out.println("imc = " + imc); // should still print 1 if immutability works

     imc.getmClass().setId(3);

     System.out.println("imc = " + imc); // should still print 1 if immutability works
}
sstan
  • 35,425
  • 6
  • 48
  • 66
  • Added more details so that you have both a working copy-constructor way of achieving immutability, but also a working `clone` way of doing it. – sstan Jul 30 '15 at 13:30
  • i have updated new modified code, not working, still getting same result @sstan – Noorus Khan Jul 30 '15 at 13:44
  • Your comment is pretty vague. Which code did you modify and to what exactly (I posted a few solutions, so I'm not sure which you are talking about)? What do you mean by "getting the same result"? – sstan Jul 30 '15 at 13:47
  • Added an edit to my answer to address your latest concern. The problem appears to be that you are testing immutability incorrectly. Have a look at the `main` method I posted. – sstan Jul 30 '15 at 15:32
0

The problem is that the clone() method of MClass is not visible in ImClass. It will work when you add the following method to MClass:

    @Override
    public Object clone() {
        try {
            return super.clone();
        } catch (Exception e) {
            return null;
        }
    }

And change your constructor to clone the object there as well (as by Jon Skeet's comment):

public ImClass(MClass mClass) {
    this.mClass = (MClass)mClass.clone();
}
Glorfindel
  • 21,988
  • 13
  • 81
  • 109
  • but is there not any way that without making any change in my other mutable class, i should make change in my immutable class only, to make the class immutable – Noorus Khan Jul 30 '15 at 12:14
  • Well, technically there might be ways. But that depends on more details than we currently have. – Marvin Jul 30 '15 at 12:19
  • @Max You can also just do the copy manually, but then you need to be sure you have access to all state information of the ImClass. – Astrogat Jul 30 '15 at 12:36
  • how to clone in the constructor ? – Noorus Khan Jul 30 '15 at 12:37
  • @Max what's not working? Do you get a compile error or does the code not produce the results you want. Please be more specific ... – Glorfindel Jul 30 '15 at 12:55
  • Exception in thread "main" java.lang.NullPointerException at com.ImClass.toString(ImmutabilityOfReferenceInstance.java:37) at java.lang.String.valueOf(Unknown Source) at java.lang.StringBuilder.append(Unknown Source) at com.ImmutabilityOfReferenceInstance.main(ImmutabilityOfReferenceInstance.java:11) – Noorus Khan Jul 30 '15 at 13:14
0

If you are trying to achieve some kind of immutable wrapper around an a mutable class, perhaps better idea would be extending it and overriding all the places where it is mutated.

class IMWrapper extends MClass {
  public IMWrapper(int id) {
     super.setId(id);
  }

  @Override
  void setId(int id) {
    throw new UnsupportedOperationException("you can't modify this instance");
  }
  ...
}
Jiri Kremser
  • 12,471
  • 7
  • 45
  • 72
0

Defensive copying is a good idea, you should just implement copy constructor for MClass:

class MClass {
    // ...

    public MClass(MClass copied) {
        this.id = copied.id;
    }
}
Dmitry Ginzburg
  • 7,391
  • 2
  • 37
  • 48
0

You already narrowed down the problem to copying/cloning an object.

You can find the solution here: How do I copy an object in Java?

Community
  • 1
  • 1
cahen
  • 15,807
  • 13
  • 47
  • 78
0

My working code

public class ImmutabilityOfReferenceInstance {

        public static void main(String[] args) {

             MClass mc = new MClass();
             mc.setId(1);

             ImClass imc = new ImClass(mc);
             System.out.println("imc = " + imc); // should print 1

             mc.setId(2);

             System.out.println("imc = " + imc); // should still print 1 if immutability works

             imc.getmClass().setId(3);

             System.out.println("imc = " + imc); // should still print 1 if immutability works
        }
    }

    final class ImClass {

        final private MClass mClass;

        public ImClass(MClass mClass) {
            this.mClass = (MClass)mClass.clone();
        }

        public MClass getmClass() {
            return (MClass)mClass.clone();
        }   

        @Override
        public String toString() {      
            return String.valueOf(mClass.getId());
        }

    }

    class MClass implements Cloneable{
        private int id;


        public int getId() {
            return id;
        }
        public void setId(int id) {
            this.id = id;
        }   

        @Override
        public Object clone() {
            try {
                return super.clone();
            } catch (CloneNotSupportedException e) {
                throw new RuntimeException(e);
            }
        }
    }
Noorus Khan
  • 1,342
  • 3
  • 15
  • 33