Validation Code

Question

I am trying to verify the validity of a string to ensure that it is a legal command I can pass to the terminal. If the string passes the test, I return True. Otherwise, I return False and an error message.

My code is pretty ugly with lots of nested if statements - how can I improve it?

task = task.split()
if len(task) > 1: 
    if task[0] == 'svn':
        if task[1] in ALLOWED:
            if len(task[2:]) == ALLOWED[task[1]]:
                return True, task, None
            else:
                return False, "Incorrect number of arguments."
        else:
            return False, "Not a legal command."    
    else:
        return False, "Not a subversion command."
else:
    return False, "Invalid input"

If by "to the terminal" you really mean "to the shell", this is a bad idea. If you've got security considerations, you're still vulnerable to shell injection. Make sure you're not invoking a shell, and use the subprocess module. And rather than returning a bool, you probably want to raise an exception. — Julian
– Julian, Commented Jul 30, 2012 at 22:54

Mark Byers · Accepted Answer · 2012-07-30 22:57:16Z

5

Instead of positive checks and nested if statements:

if a:
    if b:
        if c:
            foo()
        else:
            # error 3
     else:
         # error 2
else:
    # error 1

You can reverse the logic and bail out unless everything is OK:

if not a:
    # raise an exception

if not b:
    # raise an exception

if not c:
    # raise an exception

# If we get here, everything is OK.
foo()

This makes it easier to see which error message matches with which condition.

answered Jul 30, 2012 at 22:57

Mark Byers

844k202 gold badges1.6k silver badges1.5k bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

Joel Cornett Over a year ago

I'm going to append my answer to yours, if that's alright. Since mine is basically an example of yours.

Mark Byers Over a year ago

@JoelCornett: You can leave yours. I just +1ed it. You might even get the accept...

Joel Cornett Over a year ago

Appreciate it. Incidentally, in my code, I use if...elif...elif...else, while you do if...if...if. As a matter of style, is there a reason to prefer one over the other?

aneroid Over a year ago

+1ed both. Any thoughts on using not? So it becomes if **not**` rule_for_valid` rather than having to invert each rule (like len > 1 to len < 2). Even if one is more readable than the other? Ex: if not task[0] == 'svn' vs if task[0] != 'svn'

aneroid · Accepted Answer · 2012-07-31 01:07:47Z

2

Here is an example of how Mark Byer's answer could be implemented for your case specifically:

task = task.split()
if len(task) < 2:
    return False, "Invalid input"
if task[0] != 'svn':
    return False, "Not a subversion command."
if task[1] not in ALLOWED:
    return False, "Not a legal command."    
if len(task[2:]) != ALLOWED[task[1]]:
    return False, "Incorrect number of arguments."  
return True, task, None

edited Jul 31, 2012 at 1:07

aneroid

16.7k3 gold badges42 silver badges77 bronze badges

answered Jul 30, 2012 at 23:00

Joel Cornett

24.8k9 gold badges69 silver badges90 bronze badges

Collectives™ on Stack Overflow

Validation Code

2 Answers 2

4 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

4 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related