Returns an iterator of a private list (in a class) is considered bad practice?

Question

Suppose I have two classes: Animal and Zoo, which have a private list containing instances of Animal.

The reason I want to return the iterator is to avoid defining setters and methods for getting and removing.

Does it break encapsulation?

class Zoo{ private List<Animal> animalList; public Zoo(){ animalList = new ArrayList<Animal>(); } public void addAnimal(Animal animal){ animalList.add(animal); } public Iterator<Animal> iterator(){ return animalList.iterator(); } } class Animal{ private String name; private double weight, height; Animal(String name, double weight, double height){ this.name = name; this.weight = weight; this.height = height; } } 

Answer 1

Iterator is very rarely used outside the Iterable interface. I would advise against this practice.

I think this would be better:

 public Iterable<Animal> animals(){ return Collections.unmodifiableList( animalList ); } for(Animal a : zoo.animals()) { //do something } 

I am against Zoo implements Iterable<Animal> ; do not enter unnecessary type relationships.

In Java 8, a more preferable practice is probably to use Stream instead of Iterable.

 public Stream<Animal> animals(){ return animalList.stream(); } zoo.animals().forEach( ... 

Answer 2

Although there are situations where returning an iterator would be acceptable, in this particular case, the iterator() method breaks encapsulation because the class provides a way to mutate animalList .

As a result, the code that receives the iterator and mixes the iterations with the calls to addAnimal throws an exception.

Answer 3

Yes, it destroys encapsulation. The ArrayList iterator has a remove() method.

  Zoo zoo = new Zoo(); // ..... for (Iterator<Animal> i = zoo.iterator(); i.hasNext(); ) { i.remove(); } 

Better to offer a return immutable list.

  public List<Animal> animalList() { return Collections.unmodifiableList(animalList); } 

Answer 4

You can make your class iterable:

 public class Zoo implements Iterable<Animal> { ... public Iterator<Animal> iterator() { return animalList.iterator(); } } 

Then you can just do something like this:

 for (Animal a : zoo) { ... } 

If you return an iterator, you should do it like this:

 for (Animal a : zoo.iterator()) { ... } 

What is redundant.

---

You can also write your own iterator so that the user does not call iterator.remove() and does not modify your list:

 public class ReadOnlyAnimalIterator implements Iterator<Animal> { private Iterator iter; public ReadOnlyIterator(List<Animal> list) { this.iter = list.iterator(); } public boolean hasNext() { return iter.hasNext(); } public Animal next() { return iter.next(); } public void remove() { throw new UnsupportedOperationException(); } } 

Then in the iterator() method:

 return new ReadOnlyAnimalIterator(list); 

---

So my answer to this question is that it would be better to make your zoo iterable, perhaps by overriding Iterator if you want to make it read-only.

Answer 5

The reason I want to return the iterator is to avoid defining setters and methods for getting and removing.

There is no good reason why you should not add these methods to your class. Your code definitely violates encapsulation, but we'll come to that later. At this point, it's safe to say that Zoo breaking the correct OO design rule known as Tell Don't Ask .

With your current implementation, the client code will look like this:

 zoo.iterator().remove(); zoo.iterator().next().getName(); 

The above code is really not readable. It would be ideal to have something like this:

 zoo.removeLastAnimal(); zoo.getNextAnimalName(); 

The Zoo class can be changed as follows:

 class Zoo{ private List<Animal> animalList; Iterator<Animal> iterator; public Zoo(){ animalList = new ArrayList<Animal>(); iterator = animalList.iterator(); } public void removeLastAnimal() { try { iterator.remove(); } catch(IllegalStateException e) { //handle exception } } public String getNextAnimalName() { if(iterator.hasNext()) { return iterator.next().getName(); } } } 

This way you will hide implementation details from the outside world. You will prevent errors when using the client code code. You will be able to handle exceptions, instead of asking the client code to handle them. This is what you get with good encapsulation .