Code Smell of the Week, Design Patterns

Code Smell of the Week : Broken Encapsulation

Lets take a scenario of a customer placing an order on an ecommerce website. The order may comprise of several products. This order once placed cannot be changed but it will need to be displayed to the customer

public class Order
{
 private List<Product> products;
 //other variables, methods and constructors

 public List getProducts()
 {
  return products;
 }

 public void setProducts(
    List<Products> products)
 {
  this.products=products;
 }
}
//sample usage of Order
displayOrders(orders.getProducts());

However comparing the requirement to the code it is evident that something is wrong.

 Another developer using the Order class may write

orders.getProducts().clear();

or

orders.setProducts(null);

which will invalidate the requirements completely.

The problem arises out of the fact that the products list is exposed through the getter. One solution is

public class Order
{
 private List<Product> products;
 //other variables, methods and constructors
 public Iterator<Product> getProductsIterator()
 {
  return products.iterator();
 }
}
//usage
displayOrders(order.getProductsIterator());

Here we do not provide direct access to the list through the getter,setters but through an Iterator with modifications to the displayOrders method.
The first example is a case of Broken Encapsulation. Why is it Broken encapsulation ? It’s because we have tried to hide the list through “private” and yet we are giving access to it via the getter/setter. Thus we are giving conflicting instructions to the developer using this class.

If the list was to be readable and writable it should have been public instead of private. That would have made the Intent very clear that we are not trying to hide (encapsulate) it in the first place. So there would have been no question of broken encapsulation or confusing intent.

Thus broken encapsulation underlines a common theme in good programming, that the Intent of the code should be crystal clear.

Coming back to the solution, if you were observing carefully you would have noticed that the iterator can still access the individual product’s reference.

Iterator<Product> productsIterator= order.
  getProductsIterator();
while (productsIterator.hasNext()) {
 Product product = (Product)
 productIterator.next();
 product.price = 10;
}

That will also break the encapsulation. Then the Product class can be made immutable by removing the setters of the Product class and all classes within it.

e.g.

public class Product {
 private int price;

 private String name;

 public Product(String name,int price) {

  this.name=name;
  this.price=price;
 }

 public String getName() {
  return this.name;
 }

 public int getPrice() {
  return this.price;
 }
}

If the Product class is not editable, (from a third-party) then the final alternative is to deep copy the entire list and return the copied list’s iterator.

Thats enough to ponder on until next time ! Keep sniffing folks.

Code Smell of the Week is a blog series where I attempt to de-mystify a code smell every week and have some fun along the way.

All examples are coded in JAVA owing to its popularity.

Advertisements
Standard

10 thoughts on “Code Smell of the Week : Broken Encapsulation

  1. I have a class A in my java code whose fields I don’t want to be changed so I made them private.But I want class B to extend class A ,and use those private fields in it,but it is not possible in java?What should I do then?

  2. @Sourabh:
    So, to avoid broken encapsulation, you mean to say we should create a new object every time instead of writing setters for that class/object whenever we need to change its values ? I think this would be hectic if there are large number of attributes for an object, and if we are using sessions in web app then it may lead to problems.

    • Hi Chinmay,
      Thanks for your comment. If you want a field to be modifiable then make it public and optionally introduce the setter. That doesn’t break the encapsulation. But if changing that value changes the entire meaning of the object then you need to start thinking about Immutability. It seems a post on that is required. Keep watching this space 😉

  3. See on the line of firsts point… there has to some use of getter-setter.

    assume you have an app(steve’s jargon for application 😛 )
    your app has native monetary unit say “gold” (as in fb games)

    now

    private object gold;

    public object getgold(){
    return gold*userbaseUnit[];
    //may be us dollars or INR
    }

    public setgold(somegold){
    gold = (object) somegold/ userbaseUnit[] ;
    }

    so here basically u arent exposing the private variable directly.

    • But you are breaking the encapsulation, at-least in JAVA. Don’t know about C# which is your area. 🙂
      in JAVA when you say

      gold=(object) someGold/userBaseUnit

      I can essentially pass any value as gold and make myself rich 🙂

  4. Another point ,
    using a getter-setter for just giving access to a data member, is itself fuzzy, may be in that case the data shouldn’t be private at first place.

    the use, for instance in this scenario maybe(assuming and giving a case)

    private List products;
    public List getProducts()
    { return products;
    }
    public void setProducts( List products)
    { this.products=products;
    }

    will find place as in like this

    private List products;
    public List getProducts()
    { //Log something
    //or return only few fields(selective)
    var somepricelist;
    foreach( product return products)
    {somepricelist+= product.price;
    //or object with few data from product
    }
    return somepricelist;
    }
    public void setProducts( List products)
    {// same as getter
    // setting only few members of product in products
    //or logging over something
    this.products=products;
    }

    #sourabh your take?

    • Hi Anubhav,
      Thanks for your comment. I completely agree on your first point. If the variable is to be exposed via the getter and setters then might as well make it public and clear the intent.
      I have not understood your second part completely, but in general getter/setters do not involve computation. The methods in your code should probably be renamed.
      Can you clarify on that ?

What do you think ?

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s