I was a TA for a first-year Java programming course this year. As a very simple example of iterables/iterators, I wrote the following code for the students. I am curious if any styling or other improvements can be made.
import java.util.NoSuchElementException;
import java.util.Iterator;
public class Range implements Iterable<Integer> {
private int start, end;
public Range(int start, int end) {
this.start = start;
this.end = end;
}
public Iterator<Integer> iterator() {
return new RangeIterator();
}
// Inner class example
private class RangeIterator implements
Iterator<Integer> {
private int cursor;
public RangeIterator() {
this.cursor = Range.this.start;
}
public boolean hasNext() {
return this.cursor < Range.this.end;
}
public Integer next() {
if(this.hasNext()) {
int current = cursor;
cursor ++;
return current;
}
throw new NoSuchElementException();
}
public void remove() {
throw new UnsupportedOperationException();
}
}
public static void main(String[] args) {
Range range = new Range(1, 10);
// Long way
Iterator<Integer> it = range.iterator();
while(it.hasNext()) {
int cur = it.next();
System.out.println(cur);
}
// Shorter, nicer way:
// Read ":" as "in"
for(Integer cur : range) {
System.out.println(cur);
}
}
}
6 Answers 6
Variables
I understand why you have the Range.this.end
and Range.this.start
to avoid confusion about where those variables come from... If you need the Range.this
as part of the teaching exercise, then sure. Otherwise, I would recommend three things....
- add
range
as a prefix, even though it is slightly redundant - Make them final...
- one variable per line... (it makes revision-control diffs/patches easier to read)
The code would look like:
private final int rangeStart;
private final int rangeEnd;
Then, all the Range.this.start
would be just rangeStart
, etc.
Nested classes
Your iterator class is a non-static class, so it can reference the outer class's range start/end.
In this case, the nested class can be changed to a static class very easily. This has the potential of simplifying memory management because the iterator does not need a reference to the enclosing Range.
Consider a private-static Iterator instance:
// Inner class example
private static final class RangeIterator implements
Iterator<Integer> {
private int cursor;
private final int end;
public RangeIterator(int start, int end) {
this.cursor = start;
this.end = end;
}
public boolean hasNext() {
return this.cursor < end;
}
public Integer next() {
if(this.hasNext()) {
int current = cursor;
cursor ++;
return current;
}
throw new NoSuchElementException();
}
public void remove() {
throw new UnsupportedOperationException();
}
}
This static class removes the need for the back-references to Range.this
entirely....
The new iterator is called simply with:
public Iterator<Integer> iterator() {
return new RangeIterator(start, end);
}
Pre-Validate
It is better to pre-validate state, than to fall-through to an error... This code:
public Integer next() { if(this.hasNext()) { int current = cursor; cursor ++; return current; } throw new NoSuchElementException(); }
would be better as:
public Integer next() {
if(!this.hasNext()) {
throw new NoSuchElementException();
}
int current = cursor;
cursor ++;
return current;
}
Post-Increment
This block can be simplified:
int current = cursor; cursor ++; return current;
to just:
return cursor++;
although I imagine this is done as an education ploy.
Integer as the example
Because of the int autoboxing I worry that Integer may not be the right choice for data type. You may want to consider a non-primitive as the data.
Autoboxing is the sort of thing that will confuse.
Conclusion
Otherwise, I don't see much in the way of problems.
-
\$\begingroup\$ Thank you. I didn't think of using
final
, it's a great idea. I also agree that a private static class would be better than an inner class, but I forgot to mention that the code was meant to provide them with an example of an inner class as well, for further exposure. The post-increment was done the longer way to avoid confusion, as students find it oddly confusing. Interesting point about using a non-primitive type. I wanted to provide an example of an iterable that was not a container (say linked list, hash set, etc.) for simplicity. Can you think of one that would not use a primitive? \$\endgroup\$Sahand– Sahand2014年04月25日 05:55:31 +00:00Commented Apr 25, 2014 at 5:55 -
4\$\begingroup\$ good points, about the "Pre-Validate", to throw in some wording: this idiom is known as "guard clause" (c2.com/cgi/wiki?GuardClause), it checks pre-conditions \$\endgroup\$mdo– mdo2014年04月25日 06:20:14 +00:00Commented Apr 25, 2014 at 6:20
Overall, it's pretty good code to learn from.
Functionality
I like that you've used the inclusive-exclusive convention for the lower and upper bounds, respectively. The rationale for that design would be an interesting discussion topic.
I suggest adding a second constructor for convenience:
public Range(int end) {
this(0, end);
}
There should probably be getters for start()
and end()
. Technically, you should override .equals()
and .hashCode()
as well, but maybe it's OK to leave them out for simplicity.
Style
As @rpg711 noted, putting the @Override
annotation everywhere would be good practice. It would also help students see which methods are mandatory parts of the interface (well, practically all of them).
JavaDoc would be a good habit to teach. At the least, document the outer class and inner class, and probably their constructors as well.
It would be more conventional to put a space after the if
, for
, and while
keywords. They look less like function calls that way.
Declaring start
and end
as final
could help emphasize to students that the Range
is immutable, and only the RangeIterator
changes state. Perhaps adding final
would alleviate some of @rolfl's concerns about the inner class referring to Range.this.start
and Range.this.end
.
In agreement with @rolfl, I would also personally prefer
@Override
public Integer next() {
if (!this.hasNext()) {
throw new NoSuchElementException();
}
// The post-increment magically takes effect _after_
// returning. This is equivalent to
//
// int value = this.cursor++;
// return value;
//
return this.cursor++;
}
... though I can understand if you choose not to burden the students with that trivia.
Test case
It would be useful to illustrate that two RangeIterators
keep state independently. Perhaps this might make a good example?
Range digits = new Range(0, 10);
for (Integer tensDigit : digits) {
for (Integer onesDigit : digits) {
System.out.format("%s%s ", tensDigit, onesDigit);
}
System.out.println();
}
@rolfl totally nailed it. Only a few nitpicks left behind:
- I'd drop all pointless comments, unless they serve a purpose when you teach this
- Add the
@Override
annotations for clarity when reading not in an IDE - Whenever you can drop
this.
fromthis.cursor
, I'd drop it - The way you use spacing around brackets does not follow well the standard. Use the reformat function of your IDE (equivalent of
Control-Shift-f
in Eclipse)
I think it's a great idea asking here before presenting to your class!
-
1\$\begingroup\$ commenting here because you mention ... the comments: I personally prefer multi-line comments to multiple // — and in my codebase I insist on at least a class level javadoc (what's this Range concept anyway?) \$\endgroup\$mdo– mdo2014年04月25日 06:27:57 +00:00Commented Apr 25, 2014 at 6:27
-
3\$\begingroup\$ and @Override does not only serve readability because it secures correct overrides with compile errors \$\endgroup\$mdo– mdo2014年04月25日 06:29:00 +00:00Commented Apr 25, 2014 at 6:29
I think some students would appreciate an example without inner classes:
Range can implement the Iterator without an inner class. You just need to reset the cursor to the start value. Here I reset cursor in the Iterator method and in the next method, once it has finished iterating through the range. It works for the examples proposed. Of course, the Iterator is not keeping the states independently, and won't work for more complex examples, but I don't need to be passing constructor arguments to an inner class.
import java.util.NoSuchElementException;
import java.util.Iterator;
public class Range implements Iterable<Integer>, Iterator<Integer> {
private int start, end, cursor;
public Range(int start, int end) {
this.start = start;
this.end = end;
}
public Iterator<Integer> iterator() {
cursor = start;
return this;
}
public boolean hasNext() {
return cursor < end;
}
public Integer next() {
if(!hasNext()) {
cursor = start;
throw new NoSuchElementException();
}
return cursor++;
}
public void remove() {
throw new UnsupportedOperationException();
}
public static void main(String[] args) {
Range range = new Range(1, 10);
// Long way
Iterator<Integer> it = range.iterator();
while(it.hasNext()) {
int cur = it.next();
System.out.println(cur);
}
// Shorter, nicer way:
// Read ":" as "in"
for(Integer cur : range) {
System.out.println(cur);
}
Range digits = new Range(0, 10);
for (Integer tensDigit : digits) {
for (Integer onesDigit : digits) {
System.out.format("%s%s ", tensDigit, onesDigit);
}
System.out.println();
}
}
}
Document domain types - Make the fundamental assumptions explicit
Without documentation, the code author cannot be certain that readers share the same understanding of the notion of range represented by the Range
class.
What is Range to begin with?
Assuming if the reader recognizes that Range
is meant to describe a mathematical interval, it's still not obvious whether this interval is closed, half-open or open, i.e. start
and end
values are inclusive or not? Some code-readers might even have a question, can start
be greater than end
?
Your Iterator
implementation hinges upon the following assumptions:
that
start
can not be greater thanend
;start
value is inclusive,end
is exclusive.
They are baked into the code as implicit notions and can be inferred only by carefully examining it.
To avoid confusion, the Range
class documentation should state its instance represents a mathematical interval [start, end)
(start
inclusive, end
exclusive), and hence start
is expected to be less or equal to end
.
Validate the input - Make illegal State irrepresentable
If someone creates an invalid Range
instance with start > end
, they might be puzzled why its iterator yields no elements. It's better to make an illegal state irrepresentable to begin with by adding validation.
One of the ways to do that is to add a static
factory method verifying the validity of received arguments and then delegating the call to the private
constructor (this way we will keep the constructor dumb and free of any logic).
Modern Java
In addition, when your students gain some basic knowledge about Iterator
, it's also worth to demonstrate how this example can be reimplemented using features of the modern versions of Java.
With Java 16 Records, immutable transparent data carriers, were introduced into JDK. Range is a perfect candidate to be a record.
And specifically for the purpose of validation, Record classes are featuring a so-called compact constructor.
public record Range(int start, int end) {
public Range {
if (start > end) {
throw new IllegalArgumentException("start should less or equal to end");
}
}
}
And in order to provide an Iterator
implementation we can leverage the Stream API:
public record Range(int start, int end) implements Iterable<Integer> {
public Range {
if (start > end) {
throw new IllegalArgumentException("start should less or equal to end");
}
}
@Override
public Iterator<Integer> iterator() {
return IntStream.range(start, end).iterator();
}
}
And here's a usage example with Iterable.forEach()
:
new Range(1, 10).forEach(System.out::println);
One-line implementation
Iterable
can be loosely defined as an object that provides a mean of traversing a sequence of elements by the means of an Iterator
.
And since Java 8 Iterable
is a functional interface, hence it can be implemented a lambda expression (or a method reference if we have a method returning an iterator).
It means that we can create an ad hoc Iterable
that will behave in the same way as an instance of Range
(i.e. we can plug it into an enhanced for
-loop, invoke forEach()
on it, or generate an iterator out of it):
Iterable<Integer> range = () -> IntStream.range(1, 10).iterator();
range.forEach(System.out::println);
The iterator
method can be written using an anonymous class, which directly accesses to the attributes it needs for its job.
@Override
public Iterator<Integer> iterator() {
return new Iterator<Integer>() {
private int cursor = start;
@Override
public boolean hasNext() {
return cursor < end;
}
@Override
public Integer next() {
if (! hasNext()) {
throw new NoSuchElementException();
}
return cursor++;
}
};
with the usual tradeoffs of using anonymous/inner/separate classes.
@Override
annotations? \$\endgroup\$