Re: Code Review

Path	csiph.com!x330-a1.tempe.blueboxinc.net!usenet.pasdenom.info!news.albasani.net!fu-berlin.de!uni-berlin.de!not-for-mail
From	Ulrich Eckhardt <ulrich.eckhardt@dominolaser.com>
Newsgroups	comp.lang.python
Subject	Re: Code Review
Followup-To	comp.lang.python
Date	Wed, 25 May 2011 10:06:39 +0200
Lines	63
Message-ID	<umpua8-s08.ln1@satorlaser.homedns.org> (permalink)
References	<37ba7b40-3663-4094-b507-696fc598bf48@l26g2000yqm.googlegroups.com>
Mime-Version	1.0
Content-Type	text/plain; charset="UTF-8"
Content-Transfer-Encoding	8Bit
X-Trace	news.uni-berlin.de R+lr84t77ekV8vofR0o/fAKpEV3Top90kDwPfNT8q9GQ==
X-Orig-Path	satorlaser.homedns.org!not-for-mail
User-Agent	KNode/4.4.7
Xref	x330-a1.tempe.blueboxinc.net comp.lang.python:6207

Followups directed to: comp.lang.python

Show key headers only | View raw

ad wrote:
> Please review the code pasted below. I am wondering what other ways
> there are of performing the same tasks.

On a unix system, you would call "find" with according arguments and then 
handle the found files with "-exec rm ..." or something like that, but I see 
you are on MS Windows.

> args = parser.parse_args()
> 
> dictKeys = (vars(args))

The first of these looks okay, while I don't get the additional brackets in 
the second one. Another habit I observe here is the Hungarian notation of 
prefixing the type to the name and using camelCaps. PEP 8 (IIRC) has 
something to say on the preferred naming. I'm not 100% against encoding the 
type in the variable name in Python, since it lacks static type checking, I 
would have chosen "key_dict" here though, or, due to the small size of the 
overall program just "keys".

> print (HowManyDays)

This puzzled me at first, again the useless additional brackets I thought. 
However, in Python 3, "print" is a function, so that is correct. Still, it 
should be "print(foo)" not "print (foo)".

> for files in DirListing:
> 
>     # Get the absolute path of the file name
>     abspath = (os.path.join(WhatDirectory, files))

"files" is just the name of a single file, right? In that case the name is a 
bit confusing.

>     # Get the date from seven days ago
>     WeekOldFileDate = CurrentTime - DaysToDelete

You are repeating this calculation for every file in the loop.

>     if FileCreationTime < WeekOldFileDate:
>         #check if the object is a file
>         if os.path.isfile(abspath): os.remove(abspath)
>         # It is not a file it is a directory
>         elif os.path.isdir(abspath): shutil.rmtree(abspath)

I'm not sure, but I believe you could use shutil.rmtree() for both files and 
directories. In any case, be prepared for the file still being open or 
otherwise read-only, i.e. for having to handle errors.

Also, what if a directory is old but the content is new? Would this cause 
the non-old content to be deleted?

Cheers!

Uli

-- 
Domino Laser GmbH
Geschäftsführer: Thorsten Föcking, Amtsgericht Hamburg HR B62 932

Thread

Code Review ad <adsquaired@gmail.com> - 2011-05-24 13:10 -0700
  Re: Code Review Peter Otten <__peter__@web.de> - 2011-05-25 09:22 +0200
  Re: Code Review Chris Torek <nospam@torek.net> - 2011-05-25 07:37 +0000
  Re: Code Review Ulrich Eckhardt <ulrich.eckhardt@dominolaser.com> - 2011-05-25 10:06 +0200
    Re: Code Review ad <adsquaired@gmail.com> - 2011-05-25 06:44 -0700
      Re: Code Review Iain King <iainking@gmail.com> - 2011-05-25 07:26 -0700

csiph-web