NEVER make instance fields public

One of the most important factors that distinguish a well designed module from poorly designed ones is the degree which the module hides its internal data and implementation details from other modules. This is one of the guiding principles for encapsulation

Minimize ripple effects by hiding information

As a general rule of thumb when creating new methods or members initially make them as inaccessible as possible. Later if required to change the access; take a step back and think closely at your design. IMHO it is always a good idea to declare instance fields as private unless there is a good reason for not doing so.

De-encapsulate pitfalls…

So let’s examine what are the potential pitfalls by not declaring instance fields as private.

To demonstrate we are going to employ a SolarSystem.class. This simple class contains a static list of planets and two methods; one to display the planets displayPlanet the other getDwarfPlanet to return the dwarf planet of ‘Pluto’.

public class SolarSystem
{
  // The instance variable is should NEVER be made.
  public static List<String> planets = new ArrayList<String>();

  static
  {
    planets.add("Mercury");
    planets.add("Venus");
    planets.add("Earth");
    planets.add("Mars");
    planets.add("Jupiter");
    planets.add("Saturn");
    planets.add("Uranus");
    planets.add("Neptune");
    planets.add("Pluto");
  }

  // IAU dropped 'Pluto' in August 2006 in favour of new classification of dwarf planets.
  public static String getDwarfPlanet()
  {
    return planets.get(9); // Admittedly bad way of doing this.
  }

  // Displays all planets.
  public static void displayPlanet()
  {
    for (String planet : planets)
    {
      System.out.println(planet);
    }
  }

  public SolarSystem()
  {

  }

}

With the code above it’s very possible for any class in the application to remove ‘Pluto’ from the list of planets.

Because the planets field is public you have just lost the ability to constrain the values in that field. The values of planets can be changed from any other class in your application; and with larger applications you are getting yourself into a whole new world of pain.

public class Application
{

  private Application()
  {

  }

  public static void main(String[] args)
  {
    SolarSystem.displayPlanet();
    removePlanet("Pluto"); // Wait 'Pluto' isn't a planet anymore; we to remove it.

    // do some other fancy stuff ...
    // and more fancy stuff ...

    //So far so good ... but the following method call breaks.
    SolarSystem.getDwarfPlanet();

   /*
   * An IndexOutOfBoundsException is thrown: Index: 9, Size: 8
   */
  }

  // Removes a planet with a given name.
  public static void removePlanet(String name)
  {
    SolarSystem.planets.remove(name);
  }
}

Running SolarSystem.getDwarfPlanet() in this instance would result in IndexOutOfBoundsException to be thrown.

Aha use Getter methods!!

public class SolarSystem
{
  // The instance variable is should NEVER be made.
  private static List<String> planets = new ArrayList<String>();

  ....

  public List<String> getPlanets()
  {
    return planets;
  }
}

Well, not quite. A common approach you might think is to user getter and setter methods. We could declare our planet list as private and have a getter method but this doesn’t exactly solve our problem.  The getter method only returns a reference to the private field hence we can still modify the reference.

Think Encapsulation

This is more about the design choice rather than coding. The planets list contains a list of planets that are part of the internal working of the Solar System class and hence it should be declared as private. Once we’ve declared the planets list as private we have effectively removed any possibility of a class other than our SolarSystem class to change the values in our list. Although the example used is very trivial you can easily imagine a more complex situation whereby other external modules are accessing and manipulating the list of planets at free will, debugging issues becomes very hard.

2 responses

  1. Using a getter would solve the problem if it returned Collections.unmodifiableList(planets).

    On another note, SolarSystem should probably be a singleton… 🙂

    Like

    1. Thank you for your comment. Totally agree using Collections.unmodifiableList() would certainly provide a read-only access to the list; but as it stands ‘planets’ can be accessed in a static way regardless.

      SolarSystem as a singleton; certainly 😉 (Will change the code)

      Like

Leave a comment