-1

I have a class that needs to reset its internal iterator on a list whenever a new list is set:

public class ListElementReceiver implements ElementReceiver {
    private List<Element> elements;
    private Iterator<Element> elementIter;

    public void reset() {
        elementIter = elements.iterator();
    }

    public void setElements(List<Element> elements) {
        this.elements = elements;
        reset();
    }
}

It's basically just a wrapper around a list and its iterator so i can use it with my given Interface ElementReceiver. The problem i have is building the constructors for this class. Which of the two approaches is preferable?

// Approach 1: Duplicate logic, independant of Setter
public ListElementReceiver() {
    elements = new List<Element>();
    reset();
}

public ListElementReceiver(List<Element> elements) {
    this.elements = elements;
    reset();
}

//Approach 2: Make dependant on Setter
public ListElementReceiver() {
    setElements(new List<Element>());
}

public ListElementReceiver(List<Element> elements) {
    setElements(elements);
}
Luca Fülbier
  • 2,641
  • 1
  • 26
  • 43
  • Did you search thoroughly? I'm fairly sure I've seen at least one question closed as a duplicate of another that covered this ground. – T.J. Crowder Oct 07 '16 at 06:44
  • I did a quick search, but i felt like they didn't quite answer my question the way i wanted them to. All i found was "should i call setters from my constructor", but they didn't contain any logic besides trivial setting. – Luca Fülbier Oct 07 '16 at 06:47
  • I'm not (in just a few moments of searching) finding a clean duplicate. Certainly very close: http://stackoverflow.com/questions/12533247/java-setters-from-constructors, http://stackoverflow.com/questions/6104262/java-overridable-call-in-constructor – T.J. Crowder Oct 07 '16 at 06:52
  • The first one is along the lines of the ones i found. It doesn't include logic besides setting tho. The second one is close, but i didn't come across it. – Luca Fülbier Oct 07 '16 at 06:58

1 Answers1

5

From a constructor, you should not call methods (including setters) that can be overridden by subclasses. That can lead to code in the subclass being executed before the subclass is fully constructed, which can cause bugs that are difficult to track down.

It's okay to call private or final methods (provided they don't, in turn, invoke any overrideable code), or to call methods in a final class, but that doesn't look like what you have here.

You can avoid duplicate logic with your first approach by having one constructor invoke another:

public ListElementReceiver() {
    this(new ArrayList<>());
}

public ListElementReceiver(List<Element> elements) {
    this.elements = elements;
    elementIter = elements.iterator();
}
Ted Hopp
  • 232,168
  • 48
  • 399
  • 521
  • I didn't even think about that! Just the thing i needed, thank you! – Luca Fülbier Oct 07 '16 at 06:49
  • Perhaps worth adding that these methods that you should not call from constructors are named "alien methods" in classic references such as *Effective Java* and *Java Concurrency in Practice*. Knowing the term might help OP to find more information. – Andy Turner Oct 07 '16 at 06:53
  • @AndyTurner - Strictly speaking, that's probably correct terminology. But I think that would only confuse things. If you look in the literature, the term _alien method_ is almost always associated with discussions of concurrency and the issue here has nothing to do with concurrency _per se_. – Ted Hopp Oct 07 '16 at 07:00
  • It's still good to know, that this kind of problem is discussed in Effective Java. Reading the whole thing would be overkill for the project i am working on atm, but i might pick it up if i got some spare time. – Luca Fülbier Oct 07 '16 at 07:13
  • @Luca it is discussed in "Item 17: Design and document for inheritance or else prohibit it" – Andy Turner Oct 07 '16 at 16:54