Script Discussion & Critique

[hokiegal99]

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

 

[derek / nul]

here will do

 

[hokiegal99]

here will do

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

 

[mackstann]

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]

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)

 

[mackstann]

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)

See my reply

 

[Terry Reedy]

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

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?

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

I would lazily print one multiline string.

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

I would delete the print and add '\n' to the front of the prompt

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

Terry J. Reedy

 

[hokiegal99]

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?

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

 

[Bruno Desthuilliers]

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.

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:

> for root, dirs, files in os.walk(setpath):

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.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 :.... 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

 

[Peter Otten]

Err... Is this my version of Python having troubles, or could it be
possible that nobody actually took time to *test* that script ?

Python 2.3 (#1, Jul 30 2003, 11:19:43)
[GCC 3.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.['__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']
It's time to upgrade

Peter

 

[Peter Otten]

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

Choose variable names that make their intended purpose clear, e.g.
searchToken/replaceToken instead of x/y.

search = string.find(data, x)
if search >=1:

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

 

[Ganesan R]

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.

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.

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

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

 

[hokieghal99]

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(...)
...

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.