Hacker News new | ask | show | jobs
by lloeki 5115 days ago
Not worth a pull request, but personally I'd replace:

        if len(args) == 1:
            start, stop, step = 0, args[0], 1
        elif len(args) == 2:
            start, stop, step = args[0], args[1], 1
        elif len(args) == 3:
            start, stop, step = args
        else:
            raise TypeError('xrange() requires 1-3 int arguments')
with:

        map = [
                lambda args: (0, args[0], 1),
                lambda args: (args[0], args[1], 1),
                lambda args: args,
              ]
        try:
            start, stop, step = map[len(args)](args)
        except IndexError:
            raise TypeError('xrange() requires 1-3 int arguments')
It's more DRY, and it conveys the intent better.

I would not do such a change to the if step block since its pattern feels noticeably different: "open" checks fit well in a if/else, whereas bunch-of-equalities fit a dispatch map better (plus you can actually modify the map at runtime).

2 comments

That imho seems like overcomplicating a relatively straight-forward piece of code without noticeable improvement. In the original code the intent is clear from the first line ("check the number of arguments"), but in your version I have to parse 7 lines of code before I find out that. Also in your code I'm required to keep larger, more complicated state (the map array) in my head when reading it. Your version also looks like it would perform worse than the original. Imho code that looks like it performs suboptimally is code that looks ugly, even if the performance difference in reality would be negligible.
To me your version is less clearer than the explicit if calls:

* Name 'map' for a variable is a poor choice (as it has same name as the python builtin function map)

* Your version has off by one error, it doesn't give correct results when called with a single element list or if the list has three elements:

  >>>mymap = [
                lambda args: (0, args[0], 1),
                lambda args: (args[0], args[1], 1),
                lambda args: args,
              ]

  >>>args = [5]

  >>>mymap[len(args)](args)
    IndexError 
    Traceback (most recent call last)
    ....
   # This shouldn't be the case, a list with a single
   # element is a valid input

  >>>args = [1,6,1]

  >>>mymap[len(args)](args)
    IndexError   Traceback (most recent call last)
    ...

   # This shouldn't be the case, a list with a three
   # elements is a valid input
If only lambda's allowed statements :(.

  >>>mymap = [
               lambda args: raise IndexError,
               lambda args: (0, args[0], 1),
               lambda args: (args[0], args[1], 1),
               lambda args: args,
             ]
That aside, this approach would be much slower too.