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.

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;
        }
    }
}
share|improve this question
3  
While it is tempting to edit and improve your question, it also makes the existing answers look wrong - it invalidates them. There is a meta post about this here.... I have rolled-back your edits, but feel free to create a new question with the revised code, or one of the other alternatives suggested in that link. –  rolfl Mar 27 at 21:08
    
The edits contained clear comments indicating the changes and acknowledging the answer that prompted them. I feel it is a shame that you felt it necessary to remove them. –  Highland Mark Mar 27 at 22:33
1  
@HighlandMark: Here is good explanation about the drawbacks (and guides about the suggested ways): meta.codereview.stackexchange.com/q/1482/7076 –  palacsint Mar 27 at 22:44
1  
You don't need to name your test methods testFoo when you use the @Test annotation. Also, given that all tests are for the Range class, I'd remove it from their names. –  David Harkness Mar 27 at 22:58
    

2 Answers 2

up vote 6 down vote accepted

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

share|improve this answer
1  
Thank you for the time you spent on this, and your very helpful feedback. 3. Great refactoring - I'll put that in. –  Highland Mark Mar 28 at 16:21
    
2. Lesson learned - when implementing an interface to check for implied contracts. Fixed. 4. Yes - much better. 5. Noted, but I don't have a domain name, and my email is prone to change. 6. I agree about noise comments, however, as I am implementing an API these methods become part of the public API for my class - should I have a javadoc comment referring to the interface? 7. Agreed. The ternary operator does not make for readable code (what possessed me?). Spaces too. –  Highland Mark Mar 28 at 16:44
1  
5. I see that the recommendation to use the reverse domain name idiom for package names (7.7 Unique Package Names) was moved in the Java Language Specification at Java 7 (to 6.1 Declarations). –  Highland Mark Mar 28 at 17:58
1  
Thanks, that's the conclusion I came to too. –  Highland Mark Mar 28 at 19:41
1  
It turns out that javadoc produces almost the same output if nothing is there! –  Highland Mark Mar 28 at 19:45

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));
share|improve this answer
    
Thanks very much for that. I'll make the change you suggest. –  Highland Mark Mar 27 at 19:45
    
Thanks for pointing me to the new java.util.stream package. –  Highland Mark Mar 27 at 20:18

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.