Script Discussion & Critique

Is there a forum where one could post a Python script and have it
critiqued by others?

Something like: Y would be more efficent if you did it this way, or
doing it that way could cause problems with X, etc.

Thanks!!!

here will do

On Wed, 27 Aug 2003 18:33:21 -0400, hokiegal99 <hokiegal99@hotmail.com> wrote:
[color=blue]
>Is there a forum where one could post a Python script and have it
>critiqued by others?
>
>Something like: Y would be more efficent if you did it this way, or
>doing it that way could cause problems with X, etc.
>
>Thanks!!![/color]

derek / nul wrote:[color=blue]
> here will do[/color]

OK, I have a few questions about the script at the end of this message:

Is this script written well? Is it a good design? How could it be made
better (i.e. write to a file exactly what the scipt did)? I hope to use
it to recursively find a replace strings in all types of files (binary
and text).

Thanks for any ideas, pointers or critique... most of the ideas behind
the script came from the list.

#Thanks to comp.lang.python
import os, string
print " "
print "************************************************* *****"
print " Three Easy Steps to a Recursive find and Replace "
print "************************************************* *****"
print " "
x = raw_input("1. Enter the string that you'd like to find: ")
print " "
y = raw_input("2. What would you like to replace %s with: " %x)
print " "
setpath = raw_input("3. Enter the path where the prgroam should run: ")
print " "
for root, dirs, files in os.walk(setpath):
fname = files
for fname in files:
inputFile = file(os.path.join(root,fname), 'r')
data = inputFile.read()
inputFile.close()
search = string.find(data, x)
if search >=1:
data = data.replace(x, y)
outputFile = file(os.path.join(root,fname), 'w')
outputFile.write(data)
outputFile.close()
print "Replacing", x, "with", y, "in", fname
print " "
print "**********"
print " Done "
print "**********"
print " "

Something odd that I've discovered about this script is that it won't
replace strings at the far left side of a text file *unless* the string
has one or more spaces before it. For example:

THIS (won't be replace with THAT)
THIS (will be replaced with THAT)
THIS (will be replaced with THAT)
THIS (will be replaced with THAT)

[color=blue]
> #Thanks to comp.lang.python
> import os, string
> print " "
> print "************************************************* *****"
> print " Three Easy Steps to a Recursive find and Replace "
> print "************************************************* *****"
> print " "
> x = raw_input("1. Enter the string that you'd like to find: ")
> print " "
> y = raw_input("2. What would you like to replace %s with: " %x)
> print " "
> setpath = raw_input("3. Enter the path where the prgroam should run: ")
> print " "
> for root, dirs, files in os.walk(setpath):
> fname = files
> for fname in files:
> inputFile = file(os.path.join(root,fname), 'r')
> data = inputFile.read()
> inputFile.close()
> search = string.find(data, x)
> if search >=1:
> data = data.replace(x, y)
> outputFile = file(os.path.join(root,fname), 'w')
> outputFile.write(data)
> outputFile.close()
> print "Replacing", x, "with", y, "in", fname
> print " "
> print "**********"
> print " Done "
> print "**********"
> print " "
>[/color]

Looks good, there is one small bug and a couple very minor things that
basically boil down to style and personal preference.

-----------------------------------------------------------

import os # no need for string

# this can just go in one print with triple quotes
print """
************************************************** ****
Three Easy Steps to a Recursive find and Replace
************************************************** ****
"""

x = raw_input("1. Enter the string that you'd like to find: ")

# if you just want a newline, you can just do a print by itself
print

y = raw_input("2. What would you like to replace %s with: " %x)
print

setpath = raw_input("3. Enter the path where the prgroam should run: ")
print

for root, dirs, files in os.walk(setpath):
fname = files # what's this for? fname will just get reassigned on
# the next line.
for fname in files:
inputFile = file(os.path.join(root,fname), 'r')
data = inputFile.read()
inputFile.close()
search = data.find(x) # .find is a method of str, you don't need
# the string module for it.

# bug
#if search >=1: # this won't find it at the beginning of the file.

if search >= 0: # change to this (find returns -1 when it finds
# nothing)
data = data.replace(x, y)
outputFile = file(os.path.join(root,fname), 'w')
outputFile.write(data)
outputFile.close()
print "Replacing", x, "with", y, "in", fname

# You could also do something like this to print out the centered
# messages:

print "*"*10
print "Done".center(10)
print "*"*10

-----------------------------------------------------------

HTH,
--
m a c k s t a n n mack @ incise.org http://incise.org
Dentist, n.:
A Prestidigitator who, putting metal in one's mouth, pulls
coins out of one's pockets.
-- Ambrose Bierce, "The Devil's Dictionary"

"hokiegal99" <hokiegal99@hotmail.com> wrote in message
news:3F4D4402.3060209@hotmail.com...[color=blue]
> Is this script written well? Is it a good design? How could it be[/color]
made[color=blue]
> better (i.e. write to a file exactly what the scipt did)? I hope to[/color]
use[color=blue]
> it to recursively find a replace strings in all types of files[/color]
(binary[color=blue]
> and text).[/color]

My main answer is your answer to the following: does it operate
correctly? does it produce output within the bounds of what you
consider acceptible? In particular, does it pass your test case(s)?
and are your test case(s) reasonably representative of the domain of
application?
[color=blue]
> Thanks for any ideas, pointers or critique... most of the ideas[/color]
behind[color=blue]
> the script came from the list.
>
>
> #Thanks to comp.lang.python
> import os, string
> print " "
> print "************************************************* *****"
> print " Three Easy Steps to a Recursive find and Replace "
> print "************************************************* *****"[/color]

I would lazily print one multiline string.
[color=blue]
> print " "
> x = raw_input("1. Enter the string that you'd like to find: ")[/color]

I would delete the print and add '\n' to the front of the prompt
[color=blue]
> print " "
> y = raw_input("2. What would you like to replace %s with: " %x)
> print " "
> setpath = raw_input("3. Enter the path where the prgroam should run:[/color]
")[color=blue]
> print " "
> for root, dirs, files in os.walk(setpath):
> fname = files
> for fname in files:
> inputFile = file(os.path.join(root,fname), 'r')
> data = inputFile.read()
> inputFile.close()
> search = string.find(data, x)
> if search >=1:
> data = data.replace(x, y)
> outputFile = file(os.path.join(root,fname), 'w')
> outputFile.write(data)
> outputFile.close()
> print "Replacing", x, "with", y, "in", fname
> print " "
> print "**********"
> print " Done "
> print "**********"[/color]

Terry J. Reedy

On Wed, Aug 27, 2003 at 08:54:06PM -0400, hokiegal99 wrote:[color=blue]
> Something odd that I've discovered about this script is that it won't
> replace strings at the far left side of a text file *unless* the string
> has one or more spaces before it. For example:
>
> THIS (won't be replace with THAT)
> THIS (will be replaced with THAT)
> THIS (will be replaced with THAT)
> THIS (will be replaced with THAT)[/color]

See my reply :)

--
m a c k s t a n n mack @ incise.org http://incise.org
The church is near but the road is icy; the bar is far away but I will
walk carefully.
-- Russian Proverb

Terry Reedy wrote:
[color=blue]
> My main answer is your answer to the following: does it operate
> correctly? does it produce output within the bounds of what you
> consider acceptible? In particular, does it pass your test case(s)?
> and are your test case(s) reasonably representative of the domain of
> application?[/color]

Well it works now, mackstan pointed out a flaw in my logic that caused
the script to miss stings if they occured at the very front of a file.
Thanks!!!

hokiegal99 wrote:[color=blue]
> derek / nul wrote:
>[color=green]
>> here will do[/color]
>
>
> OK, I have a few questions about the script at the end of this message:
>
> Is this script written well? Is it a good design? How could it be made
> better (i.e. write to a file exactly what the scipt did)? I hope to use
> it to recursively find a replace strings in all types of files (binary
> and text).
>
> Thanks for any ideas, pointers or critique... most of the ideas behind
> the script came from the list.
>[/color]

Err... Is this my version of Python having troubles, or could it be
possible that nobody actually took time to *test* that script ?
There is a real problem on line 15:[color=blue]
> for root, dirs, files in os.walk(setpath):[/color]

on Python 2.2.2, here is what I get :

[laotseu@localhost dev]$ python rfp.py
(snip)
Traceback (most recent call last):
File "rfp.py", line 15, in ?
for root, dirs, files in os.walk(setpath):
AttributeError: 'module' object has no attribute 'walk'
[3]+ Done emacs testrfp
[laotseu@localhost dev]$

Of course, 'walk' is in os.path, not in os.

Python 2.2.2 (#2, Feb 5 2003, 10:40:08)
[GCC 3.2.1 (Mandrake Linux 9.1 3.2.1-5mdk)] on linux-i386
Type "help", "copyright", "credits" or "license" for more information.[color=blue][color=green][color=darkred]
>>> import os
>>> os.walk[/color][/color][/color]
Traceback (most recent call last):
File "<stdin>", line 1, in ?
AttributeError: 'module' object has no attribute 'walk'

Now a another problem on the same line :[color=blue][color=green][color=darkred]
>>> for root, dirs, files in os.path.walk(setpath):[/color][/color][/color]
.... print root, dirs, files
....
Traceback (most recent call last):
File "<stdin>", line 1, in ?
TypeError: walk() takes exactly 3 arguments (1 given)

Of course, this is not the syntax nor the way to use os.path.walk

I checked the os module for a function with similar syntax and usage,
and could not find one. os.listdir would have been a candidate, but it
does not recurse, and do not return a (root, dirs, files) tuple but a
list of filenames.

So what ?

Bruno

Bruno Desthuilliers wrote:
[color=blue]
> Err... Is this my version of Python having troubles, or could it be
> possible that nobody actually took time to *test* that script ?[/color]

Python 2.3 (#1, Jul 30 2003, 11:19:43)
[GCC 3.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.[color=blue][color=green][color=darkred]
>>> import os
>>> dir(os.walk)[/color][/color][/color]
['__call__', '__class__', '__delattr__', '__dict__', '__doc__', '__get__',
'__getattribute__', '__hash__', '__init__', '__module__', '__name__',
'__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__',
'__str__', 'func_closure', 'func_code', 'func_defaults', 'func_dict',
'func_doc', 'func_globals', 'func_name'][color=blue][color=green][color=darkred]
>>>[/color][/color][/color]

It's time to upgrade :-)

Peter

hokiegal99 wrote:
[color=blue]
> Thanks for any ideas, pointers or critique... most of the ideas behind
> the script came from the list.[/color]

Choose variable names that make their intended purpose clear, e.g.
searchToken/replaceToken instead of x/y.
[color=blue]
> search = string.find(data, x)
> if search >=1:[/color]

This is wrong as others have already explained. I recommend:

if searchToken in data:
# perform replacement

(I did it with .find() till yesterday, but no more! I like that Python
minimizes the need of artificial indices:-)

Factor out reusable code, e.g:

def replaceTokenInTree(rootFolder, searchToken, replaceToken,
filterFunc=None)
def replaceTokenInFile(filename, searchToken, replaceToken, backupFile=None)

This will pay off as your scripts grow (or your script grows:-), so make it
a habit as soon as possible.

Some observations that are not directly code-related:

You can change - and possibly corrupt - *many* files in one run of you
script. So you might add a confirmation prompt repeating searchToken,
replaceToken and the root directory (resolved to an absolute path) and -
somewhat more ambitious - a means to generate backups.

When replacement fails on one file, e. g. due to access rights, you could
catch the exception and prompt the user, if he wants to continue or abort
the script.

Chances are that there are some files in the tree that you do not want to
process, so you should add some basic filtering. The fnmatch module might
be helpful.

Last but most important:

Take time to build a test file tree, and include all special cases you can
think of (including those you consider irrelevant) e. g. files
starting/ending with search token, zerolength file, file with readonly
access.

Peter

>>>>> "hokiegal99" == hokiegal99 <hokiegal99@hotmail.com> writes:

[color=blue]
> Is this script written well? Is it a good design? How could it be made
> better (i.e. write to a file exactly what the scipt did)? I hope to
> use it to recursively find a replace strings in all types of files
> (binary and text).[/color]
[color=blue]
> Thanks for any ideas, pointers or critique... most of the ideas behind
> the script came from the list.[/color]

Others have done an excellent job of telling you the problems of critiquing
your code. I'll try to address one point that others haven't covered
already.
[color=blue]
> print " "
> print "************************************************* *****"
> print " Three Easy Steps to a Recursive find and Replace "
> print "************************************************* *****"
> print " "
> x = raw_input("1. Enter the string that you'd like to find: ")
> print " "
> y = raw_input("2. What would you like to replace %s with: " %x)
> print " "
> setpath = raw_input("3. Enter the path where the prgroam should run: ")[/color]

What you've done is not wrong. But it's simpler to replace all that with

import sys
x = sys.argv[1]
y = sys.argv[2]
setpath = sys.argv[3]

so that you can run it as "python myscript.py <search> <replace> <path>".
This way you can easily call your script from another program and
makes it much more useful. You can do something like

if len(sys.argv) >= 1:
x = sys.argv[1]
y = sys.argv[2]
setpath = sys.argv[3]
else:
raw_input(...)
...

and get the best of both worlds.

Ganesan

--
Ganesan R

Ganesan R wrote:[color=blue]
> Others have done an excellent job of telling you the problems of critiquing
> your code. I'll try to address one point that others haven't covered
> already.
>
> What you've done is not wrong. But it's simpler to replace all that with
>
> import sys
> x = sys.argv[1]
> y = sys.argv[2]
> setpath = sys.argv[3]
>
> so that you can run it as "python myscript.py <search> <replace> <path>".
> This way you can easily call your script from another program and
> makes it much more useful. You can do something like
>
> if len(sys.argv) >= 1:
> x = sys.argv[1]
> y = sys.argv[2]
> setpath = sys.argv[3]
> else:
> raw_input(...)
> ...[/color]

I don't understand this approach to developing a script. I think this is
more of a true program than a script... it's much more complex. My
background is shell scripting so I'm limited to what I've learned from
that. Python is much deeper than that, but I'm learning. Thank you for
the advice.