how to make the below code look better

[Chris Angelico]

On Wed, Dec 2, 2015 at 11:11 PM, Ganesh Pal <ganesh1pal at gmail.com> wrote:
> I need suggestion to improve the below code , Iam on Linux and python 2.7
>
> if not os.path.ismount("/tmp"):
>            sys.exit("/tmp not mounted.")
> else:
>      if  create_dataset() and check_permission():
>      try:
>           run_full_back_up()
>           run_partial_back_up()
>     except Exception, e:
>             logging.error(e)
>             sys.exit("Running backup failed")
>     if not validation_errors():
>         sys.exit("Validation failed")
>     else:
>         try:
>             compare_results()
>         except Exception, e:
>                logging.error(e)
>                sys.exit("Comparing result failed")
>
> Question 1:
>
> 1.  if  create_dataset() and check_permission():
>     Iam assuming that if statement will be executed only if both the
> functions are true ?

If both the functions return true values, yes. You have an indentation
error there, but I'm assuming you meant to have the try/except
indented further.

> 2. Can I have a if statement within  if else ? , some how I feel its messy

Certainly you can! However, most (maybe all) of your 'if' statements
are checking for error conditions, so the easiest solution is to
simply exit right away (either with sys.exit or by raising an
exception).

> 3.  Any other suggestion ? please

The first suggestion I'd make is to avoid the comma syntax for
exception handling. Replace "except Exception, e:" with "except
Exception as e:". That's a trivial change of syntax that shouldn't
affect your code at all; consider it low-hanging fruit.

The second thing to look at is the way you're 'handling' those errors.
All you do is log the error and exit. So you can skip the except
clauses altogether, and just do this:

if not os.path.ismount("/tmp"):
     sys.exit("/tmp not mounted.")
if create_dataset() and check_permission():
    run_full_back_up()
    run_partial_back_up()
if not validation_errors(): # CHECK ME
    sys.exit("Validation failed")
compare_results()


Check the logic of validation_errors, though. If it returns something
True, does that mean there were errors or there weren't? If it means
there were errors, then the 'not' seems to me to be wrong; but if it
means there weren't, then I would rename it "validation_successful"
rather than "validation_errors".

Also worth checking: If you can't create the data set or the
permission check fails, what should it do? Should it terminate with an
error? Currently, it carries on and does the validation, which is
probably not right. If it ought to terminate straight away, you can
write the whole program in "fail and bail" style:

if not create_dataset():
    sys.exit("Data set creation failed")
if not check_permission():
    sys.exit("Permission check failed")

Even better: Make those functions raise exceptions on failure, instead
of returning False. They can give extra information that way, and all
your main routine needs to do is:

create_dataset()
check_permission()

To advise further, we'd need to see what those functions are doing.
The main routine you have here is pretty simple, and anything you do
is probably going to be fine.

ChrisA