current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

Review this code.

mode = "PARALLE"

try:

  if mode == "PARALLEL":

    # some code  - this will not raise ValueError

  elif mode == "SERIES":

    # some code  

  else:

    raise ValueError("Error: Invalid mode")

except ValueError, Argument:

  print Argument


phase = "inverte"

try:

  if phase == "inverted":

    # some code

  elif phase == "non_inverted":

    # some code

  else:
      raise ValueError("Error: Invalid phase")

except ValueError, Argument:

  print Argument
share|improve this question
    
Do you expect # some code to raise ValueError sometimes? –  Janne Karila Feb 28 '14 at 6:44
    
@JanneKarila no #some_code it will not raise ValueError, only if and else will raise –  pythonlearner Feb 28 '14 at 6:54
2  
There's not much to review here. Can you provide the actual code and an explanation on what you are trying to achieve ? –  Josay Feb 28 '14 at 8:55
    
@Josay I just want to catch a ValueError in a module raised from another module –  pythonlearner Feb 28 '14 at 8:58

1 Answer 1

up vote 1 down vote

Enums and typos

mode = "PARALLE"

Typo! To avoid this, you should use a variable: PARALLEL = 1 or PARALLEL = 'PARALLEL' and mode = PARALLEL. Any typo will then be catched by Python when running the program. The uppercase is just a convention to say it's a constant. Unfortunately Python enums will only be usable in Python 3.4 which is will only be available in three weeks. And Python 3 is incompatible with Python 2, so it may not be practical to switch.

Exceptions

try:

  if mode == "PARALLEL":

    # some code  - this will not raise ValueError

  elif mode == "SERIES":

    # some code  

  else:

    raise ValueError("Error: Invalid mode")

except ValueError, Argument:

In Python, by convention we have CONSTANTS, variables, and Classes. Since your argument is only a variable, you should name it argument and not Argument. Actually, e for exception is the convention, you could stick with it (except ValueError, e:).

  print Argument

Keep it simple

What do you want to do when the mode is invalid? Who is going to see the error message? Is it acceptable to crash the application to make the error noticeable? Or do you want to try recovering in another function which called this code?

  • If you only want to print the message, then you should keep it simple:

    if mode == 'PARALLEL':
    # some code - no ValueError
elif mode == 'SERIES':
    # some other code - no ValueError
else:
    print 'Error: Invalid mode'

    You could also print the message to standard error (print >> sys.stderr, 'Error: Invalid mode' in Python 2).

  • If you want the error to be noticed and corrected (eg. if you are the only user of your program), then fail fast by raising the exception and never catching it.

  • If, however, you want to do something better, then the exception would allow you to catch the error elsewhere.

(The same comments apply to the second phase).

share|improve this answer

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.