Question about encapsulation with example question

Question:

Class SmartPhone, given below, violates the rule on encapsulation. Re-write it so that encapsulation is preserved.

class Battery{
    private String type;
    private int voltage;
    public Battery(String t, int v){type = t; voltage = v}
    public String getType(){return type;}
    public int getVoltage(){return voltage;} 
    public void setType(String t){type = t;}
    public void setVoltage(int v){voltage = v;}
}

class SmartPhone {
    private Battery battery;
    private String make, model;
    public SmartPhone(String make, String model, Battery battery){
        this.make = make; this.model = model;
        this.battery = battery;
    }
    public String getMake(){return make;} 
    public String getModel(){return model;}

    //I think here it is breaking encapsulation
    public Battery getBattery(){return battery;}
}

I can not see any other rules breaking encapsulation, I can not rember why this is breaking it.

Is it because we are returning an instance of another class from outside that class in this example itsbattery hence breaking encapsulation?

Solution

Without a clearer definition of what you mean by "the rule on encapsulation", this question is a bit ambiguous. But I think the rule is probably something like this (from Wikipedia):

Under the definition that encapsulation "can be used to hide data members and member functions", the internal representation of an object is generally hidden from view outside of the object's definition. Typically, only the object's own methods can directly inspect or manipulate its fields. Hiding the internals of the object protects its integrity by preventing users from setting the internal data of the component into an invalid or inconsistent state.

By this rule, an object should be responsible for maintaining its own state, including the state of any other objects which are its components. It should not be possible for other code to change the state directly, without using the mutator methods the object makes available in its public interface.

Accordingly, the SmartPhone class violates encapsulation because it gives out access to its internal battery component, which has setter methods. If we consider the state of the battery to be part of the internal state of the phone (because the battery is a component of the phone), then the battery's setter methods allow mutation of that internal state, not via the phone's mutator methods.

There is more than one potential solution. My preferred option is to make the Battery class immutable:

class Battery {
    private final String type;
    private final int voltage;
    public Battery(String t, int v) { type = t; voltage = v; }
    public String getType() { return type; }
    public int getVoltage() { return voltage; } 
}

This way, the getBattery method can freely return a reference to the internal battery component, safe from mutation by other code because the battery has no mutator methods.

However, the question sort of implies that the SmartPhone class is the one you are supposed to change, not the Battery class. Perhaps a battery is required to have setter methods for some other reason. In this case, the problem is a bit more subtle; the way your class is currently written, a reference to the internal battery object is available not just via the getBattery method, but also to whoever provided the battery when the object was constructed in the first place:

> Battery b = new Battery("Lithium ion", 5);
> SmartPhone s = new SmartPhone("Banana", "bPhone 2", b);
> s.getBattery().getVoltage()
5 (int)
> b.setVoltage(240); // danger! high voltage!
> s.getBattery().getVoltage()
240 (int)

Note that we are able to mutate the battery without going via s at all, because we retain a reference b to the battery we supplied.

There are two ways around this, neither of which is as neat as making the battery immutable. The first way is to hide the internal details of the Battery class completely, and make it appear that the SmartPhone class just has all four properties itself:

class SmartPhone {
    private final Battery battery;
    private final String make, model;

    public SmartPhone(String make, String model, String bType, int bVoltage) {
        this.make = make;
        this.model = model;
        this.battery = new Battery(bType, bVoltage);
    }

    public String getMake() { return make; } 
    public String getModel() { return model; }

    public String getBatteryType() { return battery.getType(); }
    public int getBatteryVoltage() { return battery.getVoltage(); }
}

This definitely doesn't allow outside access to the battery's mutator methods. Note that I've made the fields final since the class provides no way to change their values anyway.

Alternatively, we can make defensive copies to ensure that our private reference is never available to other code:

class SmartPhone {
    private final Battery battery;
    private final String make, model;

    public SmartPhone(String make, String model, Battery battery) {
        this.make = make;
        this.model = model;
        // defensive copy
        this.battery = new Battery(battery.getType(), battery.getVoltage());
    }

    public String getMake() { return make; } 
    public String getModel() { return model; }

    public Battery getBattery() {
        // defensive copy
        return new Battery(battery.getType(), battery.getVoltage());
    }
}

I don't like this solution because it misleadingly makes it appear that the battery's mutator methods will do something useful, but these don't have the advertised effects:

> Battery b = new Battery("Lithium ion", 5);
> SmartPhone s = new SmartPhone("Banana", "bPhone 2", b);
> b.setVoltage(240);
> s.getBattery().getVoltage()
5 (int)
> s.getBattery().setVoltage(240);
> s.getBattery().getVoltage()
5 (int)