Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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);
        }
    }
}
share|improve this question
2  
how 'bout @Override annotations? –  rpg711 Apr 25 at 4:20

3 Answers 3

up vote 13 down vote accepted

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....

  1. add range as a prefix, even though it is slightly redundant
  2. Make them final...
  3. 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 auto-bocxing 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.

share|improve this answer
    
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? –  Sahand Apr 25 at 5:55
3  
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 –  mdo Apr 25 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();
}
share|improve this answer

@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. from this.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!

share|improve this answer
    
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?) –  mdo Apr 25 at 6:27
2  
and @Override does not only serve readability because it secures correct overrides with compile errors –  mdo Apr 25 at 6:29

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.