Using a Pythonesque range() generator function with the Java foreach loop

Question

Now that we have the nice new foreach loop in Java, the old-style loop looks ugly be comparison.

I like the way Python has a range() generator that allows the foreach construct to iterate over a range of integers.

I have written a Range class which allows this. Please provide comments.

package highland.mark;

import java.util.Iterator;

/**
 * A class to enable java's 'foreach' loop to accept a range.
 * <p>
 * This allows replacing the C style:
 * 
 * <pre>
 * <code>
 *      for (int i = 0; i < 10; i++) {...}
 * </code>
 * </pre>
 * 
 * with:
 * 
 * <pre>
 * <code>
 *      for (int i : range(10)) {...}
 * </code>
 * </pre>
 * 
 * Three versions of range() are provided allowing combinations of start, end,
 * and step.
 * 
 * @author Mark Thomas
 * @version 1.0
 */
public final class Range implements Iterator<Integer>, Iterable<Integer> {

    /**
     * The next integer to be returned by the iterator.
     * 
     */
    private int next;
    /**
     * The last integer to be returned will be (next - 1).
     */
    private final int to;
    /**
     * The increment added to the value of next after each iteration.
     */
    private final int step;

    /**
     * A Method to be used with the java 'foreach' loop.
     * <p>
     * Usage:
     * 
     * <pre>
     * <code>
     * import static highland.mark.Range.range;
     * ...
     *      for (int i : range(from, to, step)) {...}
     * </code>
     * </pre>
     * 
     * @param from
     *            : int, first value returned.
     * @param to
     *            : int, one more than last value returned.
     * @param step
     *            : int, increment for each iteration (may be negative so long
     *            as <code>(to < step)</code>.
     * @return An Iterable<Integer> which supplies an Iterator<Integer> which,
     *         on each iteration, returns integers from <code>from</code> to
     *         <code>(to - 1)</code> incrementing by <code>step</code> each
     *         time.
     * @throws IllegalArgumentException
     *             if <code>step == 0</code> or <code>step</code> is the wrong
     *             sign.
     */
    public static Iterable<Integer> range(int from, int to, int step)
            throws IllegalArgumentException {
        return new Range(from, to, step);
    }

    /**
     * A Method to be used with the java 'foreach' loop.
     * <p>
     * Usage:
     * 
     * <pre>
     * <code>
     * import static highland.mark.Range.range;
     * ...
     *      for (int i : range(from, to)) {...}
     * </code>
     * </pre>
     * 
     * @param from
     *            : int, first value returned.
     * @param to
     *            : int, one more than last value returned.
     * @return An Iterable<Integer> which supplies an Iterator<Integer> which,
     *         on each iteration, returns integers from <code>from</code> to
     *         <code>(to - 1)</code> incrementing by 1 each time.
     * @throws IllegalArgumentException
     *             if <code>(to &lt; from)<code>.
     */
    public static Iterable<Integer> range(int from, int to)
            throws IllegalArgumentException {
        return Range.range(from, to, 1);
    }

    /**
     * A Method to be used with the java 'foreach' loop.
     * <p>
     * Usage:
     * 
     * <pre>
     * <code>
     * import static highland.mark.Range.range;
     * ...
     *      for (int i : range(to)) {...}
     * </code>
     * </pre>
     * 
     * @param to
     *            : int, one more than last value returned.
     * @return An Iterable<Integer> which supplies an Iterator<Integer> which,
     *         on each iteration, returns integers from 0 to
     *         <code>(to - 1)</code> incrementing by 1 each time.
     */
    public static Iterable<Integer> range(int to) {
        return Range.range(0, to, 1);
    }

    /*
     * (non-Javadoc) private constructor only used by the static range()
     * methods.
     * 
     * @param from : int, first value returned.
     * 
     * @param to : int, one more than last value returned.
     * 
     * @param step : int, increment for each iteration (may be negative so long
     * as (to < step).
     * 
     * @throws IllegalArgumentException if step == 0 or step is the wrong sign.
     */
    private Range(int from, int to, int step) throws IllegalArgumentException {
        if (step == 0) {
            throw new IllegalArgumentException(
                    "The step argument cannot be zero");
        }
        if ((to - from) / step < 0) {
            throw new IllegalArgumentException(
                    "The step argument has the wrong sign");
        }
        this.next = from;
        this.to = to;
        this.step = step;
    }

    /*
     * (non-Javadoc)
     * 
     * @see java.lang.Iterable#iterator()
     */
    @Override
    public Iterator<Integer> iterator() {
        return this;
    }

    /*
     * (non-Javadoc)
     * 
     * @see java.util.Iterator#hasNext()
     */
    @Override
    public boolean hasNext() {
        return this.step < 0 ? this.to < this.next : this.next < this.to;
    }

    /*
     * (non-Javadoc)
     * 
     * @see java.util.Iterator#next()
     */
    @Override
    public Integer next() {
        int value = this.next;
        this.next += this.step;
        return value;
    }

    /*
     * (non-Javadoc)
     * 
     * @see java.util.Iterator#remove()
     */
    @Override
    public void remove() throws UnsupportedOperationException {
        throw new UnsupportedOperationException(
                "The iterator returned from range() does not support remove()");
    }
}

Test code (testng):

package highland.mark;

import static highland.mark.Range.range;
import static org.testng.Assert.assertEquals;

import org.testng.annotations.Test;

public class RangeTest {

    @Test
    public void testRangeFullySpecified() {
        String result = "";
        for (int i : range(2, 19, 3)) {
            result += i + " ";
        }
        assertEquals(result, "2 5 8 11 14 17 ");
    }

    @Test
    public void testRangeBackWards() {
        String result = "";
        for (int i : range(19, 2, -3)) {
            result += i + " ";
        }
        assertEquals(result, "19 16 13 10 7 4 ");
    }

    @Test
    public void testRangeDefaultStep() {
        String result = "";
        for (int i : range(2, 9)) {
            result += i + " ";
        }
        assertEquals(result, "2 3 4 5 6 7 8 ");
    }

    @Test
    public void testRangeDefaultStepAndStart() {
        String result = "";
        for (int i : range(7)) {
            result += i + " ";
        }
        assertEquals(result, "0 1 2 3 4 5 6 ");
    }

    @Test(expectedExceptions=IllegalArgumentException.class)
    public void testwrongWay1() {
        for (@SuppressWarnings("unused") int i : range(2, 19, -3)) {
            // No-op;
        }
    }

    @Test(expectedExceptions=IllegalArgumentException.class)
    public void testwrongWay2() {
        for (@SuppressWarnings("unused") int i : range(2, -19, 3)) {
            // No-op;
        }
    }

    @Test(expectedExceptions=IllegalArgumentException.class)
    public void testZeroStep1() {
        for (@SuppressWarnings("unused") int i : range(2, 19, 0)) {
            // No-op;
        }
    }

    @Test(expectedExceptions=IllegalArgumentException.class)
    public void testZeroStep2() {
        for (@SuppressWarnings("unused") int i : range(2, -19, 0)) {
            // No-op;
        }
    }
}

Answer 1

+1 to @rolfl and some other notes:

1. I like your finals and the self-checking unit tests. It's good that they test only one thing, it helps debugging and defect localization. Keep it up!

2. The code violates the Iterator.next()'s contract:

   Throws:

   NoSuchElementException - if the iteration has no more elements

   Consider this:

   final Iterable<Integer> range = range(1, 3, 1);
   final Iterator<Integer> iterator = range.iterator();
   System.out.println(iterator.next()); // 1
   System.out.println(iterator.next()); // 2
   System.out.println(iterator.next()); // 3, but should throw NoSuchElementException
   

   The third next() prints 3 instead of NoSuchElementException.

3. I'd move some duplication from the test method to a custom verification method:

   private <T> void verifyRange(final Iterable<T> range, final T... expectedValues) {
       final List<T> result = new ArrayList<>();
       for (T value: range) {
           result.add(value);
       }
       assertEquals(Arrays.asList(expectedValues), result);
   }
   

   Usage:

   verifyRange(range(2, 19, 3), 2, 5, 8, 11, 14, 17);
   

4. @Test(expected = IllegalArgumentException.class)
   public void testwrongWay1() {
       for (@SuppressWarnings("unused")
       int i: range(2, 19, -3)) {
           // No-op;
       }
   }
   

   You don't need the loop nor the @SuppressWarnings here, the following is the same:

   @Test(expected = IllegalArgumentException.class)
   public void testwrongWay1() {
       range(2, 19, -3);
   }
   

5. You could use a better package name than this:

   package highland.mark;
   

   Java Package Names on c2.com

6. This comment is rather unnecessary, I'd remove it:

   /*
    * (non-Javadoc)
    * 
    * @see java.lang.Iterable#iterator()
    */
   @Override
   public Iterator<Integer> iterator() {
       return this;
   }
   

   (Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)

7. This is definitely hard to read/understand:

   return this.step < 0 ? this.to < this.next : this.next < this.to;
   

   Consider this:

   if (this.step < 0) {
       return this.to < this.next;
   } else {
       return this.next < this.to;
   }
   

   Or this:

   @Override
   public boolean hasNext() {
       if (step < 0) {
           return to < next;
       } else {
           return next < to;
       }
   }
   

   Modern IDEs use highlighting to separate local variables from fields, so you don't have to use this. here.

8. /**
    * The next integer to be returned by the iterator.
    * 
    */
   private int next;
   /**
    * The last integer to be returned will be (next - 1).
    */
   private final int to;
   /**
    * The increment added to the value of next after each iteration.
    */
   private final int step;
   

   A few empty lines between the fields would be readable:

   /**
    * The next integer to be returned by the iterator.
    */
   private int next;
   
   /**
    * The last integer to be returned will be (next - 1).
    */
   private final int to;
   
   /**
    * The increment added to the value of next after each iteration.
    */
   private final int step;
   

   Anyway, these comments doesn't say anything which is not already in the code, I'd remove this ones too.

Answer 2

For the most part, this appears to be neat, and well structured.

There is a significant bug, though:

• Iterable<Integer> implies an iterator() method. Each time you call that iterator() method you should get a new instance of an interator, not an 'expired' instance.

For example, the following code should print the numbers 1 to 10 twice....

Range onetoten = Range.range(1, 10, 1);

for (Integer i : onetoten) {
    System.out.println(i);
}

for (Integer i : onetoten) {
    System.out.println(i);
}

Your code will only print them once.

The fix for this is to store the from value as well as the to and the step, and to then return a duplicate Range in the iterator() method.

---

EDIT: I should point out this: IntStream.rangeClosed() (Java8)

IntStream.rangeClosed(1, 10).forEach(i -> System.out.println(i));