Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.python > #99872 > unrolled thread

how to make the below code look better

Back to article view | Back to comp.lang.python

Contents

  how to make the below code look better Ganesh Pal <ganesh1pal@gmail.com> - 2015-12-02 17:41 +0530
    Re: how to make the below code look better BartC <bc@freeuk.com> - 2015-12-02 12:20 +0000
      Re: how to make the below code look better Chris Angelico <rosuav@gmail.com> - 2015-12-02 23:31 +1100
    Re: how to make the below code look better Steven D'Aprano <steve@pearwood.info> - 2015-12-03 00:28 +1100
      Re: how to make the below code look better Chris Angelico <rosuav@gmail.com> - 2015-12-03 00:49 +1100
    Re: how to make the below code look better me <self@example.org> - 2015-12-02 15:23 +0000

#99872 — how to make the below code look better

Hello team,

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 ?

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

3.  Any other suggestion ? please

[toc] | [next] | [standalone]

#99873

On 02/12/2015 12:11, Ganesh Pal wrote:

> 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")

> 3.  Any other suggestion ? please


You could make the indentation more consistent. Example:

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")



-- 
Bartc

[toc] | [prev] | [next] | [standalone]

#99875

On Wed, Dec 2, 2015 at 11:20 PM, BartC <bc@freeuk.com> wrote:
> You could make the indentation more consistent. Example:

You fixed one indentation error...

>     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")

... but introduced another :)

>         try:
>             compare_results()
>             except Exception, e:
>                 logging.error(e)
>                 sys.exit("Comparing result failed")

These last three lines need to be unindented one level. Otherwise,
Bart's recommendation is a good one.

ChrisA

[toc] | [prev] | [next] | [standalone]

#99880

On Wed, 2 Dec 2015 11:11 pm, Ganesh Pal wrote:

> Hello team,
> 
> 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.")

This is good enough for quick and dirty scripts, but this is vulnerable to a
race condition. It may be that /tmp is mounted *now*, but a millisecond
later (before you can use it) another process unmounts it.

This is called a "time of check to time of use" bug:

https://cwe.mitre.org/data/definitions/367.html

https://www.owasp.org/index.php/Time_of_check,_time_of_use_race_condition

https://en.wikipedia.org/wiki/Time_of_check_to_time_of_use

and can be a serious software vulnerability.

If this code is only being used under trusted conditions, then it is
probably okay, otherwise you should reconsider your strategy.

(Besides, how often do you unmount /tmp?)




> else:
>      if  create_dataset() and check_permission():

this with:

elif create_dataset() and check_permission():

but again be mindful of the risk of race conditions.



>      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")

I would swap the sense of validation_errors(). Currently, it looks like it
returns True if there are *no* errors, and False if there are errors. That
seems confusing and easy to get wrong. 

    if validation_errors():
        sys.exit("Validation failed")

reads better and is easier to understand. Now validation_errors() returns
true if there are errors, and false if there are no errors.


>     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 ?

Correct.


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

Yes you can, but elif is usually nicer, and saves a level of indentation.



-- 
Steven

[toc] | [prev] | [next] | [standalone]

#99885

On Thu, Dec 3, 2015 at 12:28 AM, Steven D'Aprano <steve@pearwood.info> wrote:
>> if not os.path.ismount("/tmp"):
>>            sys.exit("/tmp not mounted.")
>
> This is good enough for quick and dirty scripts, but this is vulnerable to a
> race condition. It may be that /tmp is mounted *now*, but a millisecond
> later (before you can use it) another process unmounts it.
>
> This is called a "time of check to time of use" bug:
>
> https://cwe.mitre.org/data/definitions/367.html
>
> https://www.owasp.org/index.php/Time_of_check,_time_of_use_race_condition
>
> https://en.wikipedia.org/wiki/Time_of_check_to_time_of_use
>
> and can be a serious software vulnerability.
>
> If this code is only being used under trusted conditions, then it is
> probably okay, otherwise you should reconsider your strategy.
>
> (Besides, how often do you unmount /tmp?)
>

Possibly it's not worried about *un*mounting of /tmp, but about being
run prior to /tmp being mounted for the first time. If that's the
case, the check/use difference won't matter - worst case, the program
errors out even though the mount was almost completed.

ChrisA

[toc] | [prev] | [next] | [standalone]

#99895

On 2015-12-02, Ganesh Pal <ganesh1pal@gmail.com> wrote:
> 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")

Apart from what already mentioned (indentation, assumptions on
environment):

> 1.  if  create_dataset() and check_permission():
>     Iam assuming that if statement will be executed only if both the
> functions are true ?

Yes, this is the meaning of 'and'.
Notice how functions are always true, but the result of the function call
might not be.

- create_dataset is a function.
- create_dataset() is a function call.

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

You are searching for `elif`.

[toc] | [prev] | [standalone]

Back to top | Article view | comp.lang.python

csiph-web