13

I was just wondering why java.util.Scanner implements java.util.Iterator?

Scanner implements the remove method and throws an UnsupportedOperationException.

But shouldn't a class, when implementing an interface, fulfill the contract of the interface?

What is the use of implementing iterator and adding a method that throws an exception?

Why not just avoid the implementation of the interface and keep it simple?

One can argue that it is defined so that the class which might extend Scanner could implement the method, like AbstractList has an add method that throws an UnsupportedOperationException. But AbstractList is an abstract class, whereas Scanner is a final class.

Is this not a bad design practice?

Vince
  • 14,470
  • 7
  • 39
  • 84
Thirumalai Parthasarathi
  • 4,541
  • 1
  • 25
  • 43
  • A related [question](http://stackoverflow.com/questions/22050848/do-collections-unmodifiablexxx-methods-violate-lsp) regarding your doubt of it throwing `UnsupportedOperationException`. As people have pointed out `remove` is an optional method which may not be implemented by concrete class. So it is not violating the documentation. But there is a principle Liskov Substitution principle which I felt it is violating. But I am not sure.. Read the question I linked for more. – Narendra Pathai Jul 01 '15 at 05:59
  • i saw your question and i believe you are right. It is indeed breaking the LSP. I also believe the class should never have implemented the iterator interface. I would be happy to accept your answer if you could post one. – Thirumalai Parthasarathi Jul 01 '15 at 06:10
  • 3
    Yes, the Java collections library was poorly designed. There should have been a read-only iterator interface, but there isn't. – Chris Martin Jul 01 '15 at 06:14

4 Answers4

11

I'd say yes, it's a design flaw. The flaw is within Iterator. This issue could be thrown in the same category as attempting to create an immutable Collection implementation.

It violates the Interface Segregation Principle and forces the developers to include a corner case into the JavaDocs (the infamous UnsupportedOperationException) to avoid violating the Liskov Subsitution Principle. You'll find this in Collection#remove methods aswell.


I believe design could be improved by decomposing the interface, segregating hasNext() and next() into a new (immutable) interface and letting the (mutable) Iterator interface derive from it :

interface Traversable<E> {
    boolean hasNext();
    E next();
}

interface Iterator<E> extends Traversable<E> {
    void remove();
}

final class Scanner implements Traversable<String> {

}

Better names could definitely be used. Please don't down this post due to my bad naming choices.

Why does Scanner implement Iterator in the first place?

Scanner is not an iterator in the sense of traversing a collection. But the idea of a Scanner is to supply it input to be "scanned", which in a sense is iterating over something (the characters in a String).

I can see why Scanner would implement Iterator (you were asking for a use case). For example, if you wanted to create your own Iterable type to iterate over a String specifying a delimiter:

class ScannerWrapper implements Iterable<E> {
    public Scanner scanner;

    public ScannerWrapper(Scanner scanner) {
        this.scanner = scanner;
    }

    public Iterator<String> iterator() {
        return scanner;
    }
} 

Scanner scanner = new Scanner("one,two,three");
scanner.useDelimiter(",");
ScannerWrapper wrapper = new ScannerWrapper(scanner);

for(String s : wrapper) {
    System.out.println(s);
}

But this would have also worked if the JDK supported a Traversable type and allowed enhanced loops to accept Traversable items, since removing from a collection in this fashion may throw a ConcurrentModificationException, which leads to using an iterator instead.

Conclusion

So is it good design? Nope. It violates ISP and results in cluttered contracts. This is simply a giagantic code smell. The real problem is the language's lack of support for immutability, which should allow developers to specify whether a behavior should mutate state, allowing behavioral contracts to be stripped of their mutability. Or something along those lines..

The JDK is filled with stuff like this (bad design choices, such as exposing length for arrays and attempts at an ImmutableMap I mentioned above), and changing it now would result in breaking code.

Vince
  • 14,470
  • 7
  • 39
  • 84
3

Because implementing iterator allows the scanner to be used wherever a read only iterator can be used.

Furthermore it does implement the contract. From the Iterator documentation (emphasis mine):

remove() Removes from the underlying collection the last element returned by this iterator (optional operation).

Eli Algranti
  • 8,707
  • 2
  • 42
  • 50
  • 1
    Scanner cannot be used in for each loop as it implements Iterator and not Iterable. – Narendra Pathai Jul 01 '15 at 05:37
  • @NarendraPathai is rite.. though the examples you have given is wrong, i still understand that in a place of a read-only iterating operation we can use it. But is there such a situation? – Thirumalai Parthasarathi Jul 01 '15 at 05:40
  • @ThirumalaiParthasarathi scanner needs to iterate through the input so it makes sense to implement an existing interface. Implementing Iterable would have been a better choice (@VinceEmigh explains it better). Still not implementing remove() is not a break of contract since this method was intended to be optional. Why is there no way to discover if an iterator is read only without incurring an exception (an isReadOnly() method would be nice) is another Java design snafu IMO. – Eli Algranti Jul 01 '15 at 12:16
  • i agree... But an optional method to be implemented in an interface, just doesn't sound right to me. Maybe i am not matured enough to understand why the designers chose so, or maybe its a design flaw as you and others have said. But thanks for you answer it showed me that the method is **optional** to implement. :) – Thirumalai Parthasarathi Jul 03 '15 at 04:58
  • @ThirumalaiParthasarathi You are correct, it isn't natural. What's the point of a contract if you aren't required follow it? An `interface` is simply a behavioral contract. Sadly, this is possible by adding domain specific corner cases to the contract, which at times is the best (or only) option. In my opinion, the design of `Scanner` is *extremely* flawed (structurally and philosophically), so I try to avoid using it. – Vince Aug 13 '16 at 22:05
1

For an official answer, see the Collections Framework Overview. Scroll down to the bottom for Design Goals.

To keep the number of core interfaces small, the interfaces do not attempt to capture such subtle distinctions as mutability, modifiability, and resizability. Instead, certain calls in the core interfaces are optional, enabling implementations to throw an UnsupportedOperationException to indicate that they do not support a specified optional operation. Collection implementers must clearly document which optional operations are supported by an implementation.

As previous answers have pointed out, the Collections framework could uphold Liskov Substition by decomposing its interfaces into numerous, smaller interfaces. That approach was considered and rejected, in order to minimize the number of core interfaces in the framework.

Community
  • 1
  • 1
jaco0646
  • 15,303
  • 7
  • 59
  • 83
  • A decision that wasn't thought through completely. As mentioned before, the collection framework was poorly designed. Their desire for a smaller API impacts us in a negative way. We should not be forced to violate design principles because a few devs feel their API would be a bit bulky. In my opinion, the amount of interfaces needed to make a legit collections framework is nothing compared to the design principles we are forced to violate. Maybe a smaller API outweighted such design principles in the past (although I doubt it). (continued in next comment) – Vince Jul 04 '15 at 20:12
  • Either way, something as common as the JDK should not force us to violate design principles, or jump through hoops, to achieve what we want. Josh Bloch is one of my idols, although I can't say he made the best decisions when it comes to the collections framework. – Vince Jul 04 '15 at 20:12
0

It's just not a supported method. To avoid implementing it that way would require a similar interface without a remove method. Is it good design to create multiple similar interfaces just to avoid a method that throws NotImplementedException?

Anyone using a Scanner knows (or will soon know) that the remove() method can't be used, so it really has no practical effect. It could also be a no-op, since effectively the item is removed, just not due to remove(), but it's probably clearer to throw an exception.

Kayaman
  • 72,141
  • 5
  • 83
  • 121
  • I agree with you on not creating multiple interfaces that have duplicate opearations, but there is always a better design, the iterator can be made to extend another interface, say `x` which specifies of hasNext and next, and the iterator can have the remove method. This way `Scanner` can implement `x`. So is this a design flaw? – Thirumalai Parthasarathi Jul 01 '15 at 05:31
  • That's not a better design at all. I don't want to have `Iterable`, `IterableWithoutRemove` and `IterableWithoutAdd` interfaces in the standard library, just because you don't like that a method throws a `NotImplementedException`. Not to mention that in addition to the multiple `Iterable` interfaces, the `Iterator` interfaces would be in triplicate too. Adding unnecessary clutter is not good design. – Kayaman Jul 01 '15 at 05:37
  • +1 Thanks for the quick response and making sense to me. I completely agree, a standard library shouldnt be stuffed in with unwanted complexity. But then what good is made by making Scanner implement Iterator. Why can it stand alone without the Iterator? Btw `IterablewithoutAdd`.. seriously?? :P – Thirumalai Parthasarathi Jul 01 '15 at 05:43
  • I suspect the reason is due to `Scanner` being used to read files line by line (although these days there are better options). A simple foreach loop can be used to go through the file, instead of a more complex while loop with a `BufferedReader`. – Kayaman Jul 01 '15 at 05:46
  • But it is not iterable as @NarendraPathai pointed out. So it cant be used in a for-each loop rite.? – Thirumalai Parthasarathi Jul 01 '15 at 05:47
  • Ah you're right. I've never really cared much for `Scanner`, since it seems to be used mainly for simple command line input reading and converting i.e. student work. It's just guessing, but I suppose they might've implemented `Iterator` just because of the `hasNextLong()` and other methods, which make the class kinda look like an `Iterator` (which of course only has `hasNext()` and `next()`). There certainly seems to be no compelling reason for the class to implement `Iterator`. – Kayaman Jul 01 '15 at 05:58
  • @Kayaman I don't want to have types that violate LSP just because you don't like long names. – Chris Martin Jul 01 '15 at 06:16
  • @ChrisMartin It's not long names, it's the redundant interfaces. It's Liskov Substitution **Principle** not Liskov Substitution Absolute Rule. I've seen people write horrible code just because they think they're doing good design by following principles without any common sense. With experience you'll realise that while it's a good idea to follow principles, there are always exceptions. – Kayaman Jul 01 '15 at 06:27
  • @Kayaman Experience has taught me that when your types are lies, you're pretty much always setting yourself up for trouble. – Chris Martin Jul 01 '15 at 06:30
  • @ChrisMartin Don't worry, you'll get more experience. – Kayaman Jul 01 '15 at 06:32
  • Besides, the issue here isn't so much the optional operations, but rather the unnecessity of `Scanner` implementing `Iterator` in the first place. As I said previously, while `Scanner` kinda looks like an `Iterator` it really isn't. – Kayaman Jul 01 '15 at 06:39