On Wed, Dec 2, 2015 at 6:00 PM, Chris Angelico <ros...@gmail.com> wrote:
> 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.
>
Correct I had meant to have 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).
Yes agreed , have included the suggestion
>> 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.
>
,Are we avoiding comma syntax because it's a bad convention or will
it have any side effects? In fact I have used this comma syntax in
most of my code . I will clean the code latter as it seems working
fine
> 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()
>
>
Agreed.
> 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")
>
Thanks for other suggestions also
--
https://mail.python.org/mailman/listinfo/python-list
