Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

Magnus Lie Hetland in his Beginning python wrote a code to replicate the range() built-in.

def interval(start, stop=None, step=1):
    if stop is None: 
        start, stop = 0, start 
    result = []
    i = start 
    while i < stop: 
        result.append(i)
        i += step 
    return result

I re-wrote it like this.

def interval(*args):    
    if  len(args)<1:
        raise TypeError('range expected at least 1 arguments, got 0')
    elif len(args)>3 :
         raise TypeError('range expected at most 3 arguments, got %d' % len(args))
    else:
        if len(args)==1:
            start = 0
            stop = args[0]
            step = 1
        elif len(args)==2:
            start=args[0]
            stop=args[1]
            step=1
        elif len(args)==3:
            start=args[0]
            stop=args[1]
            step=args[2]

        result = []
        while start < stop:
            result.append(start)
            start+=step

        return result

I know my code is lengthier but don't you people think its easier to grasp/understand than Hetland's code? Different peoples' minds work differently. In my mind the code felt easier to understand cos I come from a C background. My emphasis is on code comprehension.

share|improve this question
You got Magnus Lie Hetland's version slightly wrong. The line that reads i = start is indented one level too far. – Omnifarious Dec 15 '11 at 8:55

3 Answers

up vote 3 down vote

I think your code is more straightforward, but its size is a bit daunting. I actually like the original better. The only real 'trick' it uses is multiple assignment. And while it requires you to think through the different cases of range a bit to realize it works, this isn't something I mind. In fact, with yours, if I were to verify its correctness I'd have to spend just as much time because it has so much more code.

I tend to prefer code that's a bit terse, but elegantly handles all the cases to code that laboriously checks each case and handles that case with case-specific code. And this is in any programming language.

share|improve this answer
Thanks I get the point.less code if not complicated even if complex. – Jibin Dec 15 '11 at 8:24
up vote 3 down vote

This is bad

    while start < stop:
        result.append(start)
        start+=step

Don't reuse start to be the index of the loop. It's confusing. It violates the meaning of start.

The lengthy parameter parsing is good, but can be made more clear. Don't waste time on checking for <1 or >3. Just use the if statement.

if len(args)==1:...
elif len(args)==2:...
elif len(args)==3:...
else:
    raise TypeError( "Wrong number of arguments" )
share|improve this answer
Well,the range() built-in shows the argument No in exception message like range expected at least 1 arguments, got 0.I wanted to emulate it. – Jibin Dec 30 '11 at 4:04
Thanks for the first observation. – Jibin Dec 30 '11 at 4:39
@Jibin: "built-in shows the argument number". That doesn't mean it's good design. It should also be part of the else clause if it's so important to provide that kind of message. – S.Lott Dec 30 '11 at 13:29
:got that.I think there is a psychological reason to 'why error/exception should be in else case'.Suppose a programmer is reading the code, for first few moments his mind is fresh so main logic should come first,error/exceptions can come last.What do you think? – Jibin Jan 3 '12 at 4:32
"logic should come first,error/exceptions can come last." I'm unclear on how it can be otherwise. What's the alternative? – S.Lott Jan 3 '12 at 23:06
show 3 more comments
up vote 2 down vote

You've replaced three named arguments with a variable-length list of anonymous arguments and three seperate ways of mapping list positions onto named variables ... I'd question why that seems clearer to you.

You're then manually enforcing constraints on the variable-length argument list which the interpreter handled for you in the original code.

And, finally, where you raise an exception, you don't need to put the following code in an else block: if you're coming from C, think of the raise more like an early return.

eg.

def interval(*args):    
    if  len(args)<1:
        raise TypeError('range expected at least 1 arguments, got 0')
    if len(args)>3 :
        raise TypeError('range expected at most 3 arguments, got %d' % len(args))
    if len(args)==1:
        ...

(although S.Lott's formulation is better still).

share|improve this answer
The problem with the version that has three named arguments is that those names aren't necessarily indicative of what the argument does, i.e. if you call interval(3) the name of the first argument is start even though its semantics are stop. So for that reason alone I think the *args version is actually clearer - at least the user won't make any assumptions about what the arguments to without consulting the documentation first. – sepp2k♦ Dec 16 '11 at 22:38
OK, good point: the code itself still looks more readable to me, but might also be misleading. – Useless Dec 19 '11 at 16:31
Ok,I agree that else & elif is not required,but they make sure that only one block gets executed for sure,though logically there is no possibility for multiple if blocks getting executed in your code cos exception is raised .Is there any performance penalty if I use elif or else than if – Jibin Dec 30 '11 at 4:23

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.