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"

• Julian: 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.

Answer 1

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.

• Joel Cornett: I'm going to append my answer to yours, if that's alright. Since mine is basically an example of yours.
• Mark Byers: @JoelCornett: You can leave yours. I just +1ed it. You might even get the accept...
• Joel Cornett: 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: +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'

Answer 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