current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 7 down vote favorite

I wanted to learn about Iterable and Iterators and hence thought about getting my hands dirty rather than reading theory and forgetting it.

I am curious how I can improve readability and error handling of my code. I also thought to practice and include test cases.

class foo_range(object):
    """ Custom range iterator which mimics native xrange. """

    def __init__(self, start, stop, step=1):
        try:
            self.current = int(start)
            self.limit = int(stop)
            self.step = int(step)
        except ValueError:
            raise
        if step == 0:
            raise ValueError("Step can't be 0")

    def __iter__(self):
        return self

    def next(self):
        if self.step > 0 and self.current >= self.limit:
            raise StopIteration()
        if self.step < 0 and self.current <= self.limit:
            raise StopIteration()
        oldvalue = self.current
        self.current += self.step
        return oldvalue



import unittest

class TestXRange(unittest.TestCase):

  def test_invalid_range(self):
    with self.assertRaises(ValueError) as ve:
      foo_range(0, 5, 0)

  def test_valid_range(self):
    expected = [0, 1, 2]
    actual = []
    for _ in foo_range(0, 3):
        actual.append(_)
    self.assertEqual(actual, expected)

    expected = [0, -1, -2, -3]
    actual = []
    for _ in foo_range(0, -4, -1):
        actual.append(_)
    self.assertEqual(actual, expected)

    expected = []
    actual = []
    for _ in foo_range(0, 5, -1):
        actual.append(_)
    self.assertEqual(actual, expected)

    expected = []
    actual = []
    for _ in foo_range(0, -5, 1):
        actual.append(_)
    self.assertEqual(actual, expected)


if __name__ == '__main__':
    unittest.main()
share|improve this question
up vote 5 down vote accepted

Note that xrange is an iterable. It is possible to iterate multiple times over a single xrange instance. foo_range, by contrast, is an iterator and as such can only be iterated over once. 

>>> xr = xrange(5)
>>> list(xr)
[0, 1, 2, 3, 4]
>>> list(xr)
[0, 1, 2, 3, 4]
>>> fr = foo_range(0,5)
>>> list(fr)
[0, 1, 2, 3, 4]
>>> list(fr)
[]

To fix that, __iter__ should return a new iterator object. A simple way to accomplish that would be to make __iter__ a generator of the values. Alternatively, rename your class to something like foo_range_iterator and add a new foo_range class whose __iter__ returns a new foo_range_iterator instance.

share|improve this answer
    
What happens when I do foo_range(0, 5) second time? – CodeYogi Oct 23 '15 at 9:15
    
@CodeYogi If you do foo_range(0, 5) second time you get a new object, and iteration starts from beginning. The difference is when you try to use the same object again, like in the demo I just edited into my answer. – Janne Karila Oct 23 '15 at 10:08
up vote 6 down vote

Question

I am not sure to understand the point of having this :

    except ValueError:
        raise

List comprehension

You have the following pattern :

actual = []
for _ in foo_range(XXX):
    actual.append(_)

in a few places.

It can easily be replaced by :

actual = [_ for _ in foo_range(XXX)].

Also, _ is usually used as a variable name for throw-away values (values that will not be used). In your case, because you actually use the value, it might be better to name it (a, e, val, i, etc).

Also, I think this is equivalent to :

actual = list(foo_range(XXX))

and I'll let you consider this.

Unit test organisation

Your def test_valid_range(self): test tests multiple things. It is better to split this into multiple smaller tests. Among other things, it is easier to understand if something goes wrong but it also helps you to think about what you are actually trying to test (iterating forward, iterating backward, etc).

Suggestion

It may be interesting to try to implement this using a generator defined with yield.

share|improve this answer
    
I wanted to let the reader of my code know that the given code block is prone to error, one way was to use documentation and there define all the constraints of the argument. Anyways suggestions are always welcome. – CodeYogi Oct 15 '15 at 8:49
    
"I think this is equivalent to" yep, it is. – Barry Oct 15 '15 at 14:23
up vote 1 down vote

Missing Feature

One missing feature in all of this is probably the most common usage of xrange():

xrange(stop)
xrange(start, stop[, step])

You're missing the first case! I can't write something like:

for i in foo_range(10):
    # stuff

You should add support for that.

Missing Type Check

xrange() type checks it's input. For instance, if I tried to do xrange(10.1), it throws a TypeError. You may or may not want to add this, due to all the issues of floating point comparisons that supporting floating point ranges brings up.

Testing

You have tests for values of step that are 0, 1, and -1. You should probably have at least one test for a step value of like 5.

Stopping Iteration

This is minor, but you have:

if self.step > 0 and self.current >= self.limit:
    raise StopIteration()
if self.step < 0 and self.current <= self.limit:
    raise StopIteration()

Those aren't independent checks, which suggests that the second should at least be an elif. But since the body is the same, I'd suggest just combining them:

if (self.step > 0 and self.current >= self.limit or
    self.step < 0 and self.current <= self.limit):
    raise StopIteration()
share|improve this answer
    
Interesting! how do you know that two checks are independent? you apply some technique or its just common sense (which I lack)? – CodeYogi Oct 15 '15 at 15:36
1  
@CodeYogi self.step is either positive or negative - if it's both you've got bigger problems :) – Barry Oct 15 '15 at 15:45

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.