Re: [Tutor] code review
On 12/06/14 00:38, Alan Gauld wrote: HTH Thanks Alan and Lukáš for your very helpful comments. I will attempt to revise the script in light of them and will revert if I hit any brick walls :) ___ Tutor maillist - Tutor@python.org To unsubscribe or change subscription options: https://mail.python.org/mailman/listinfo/tutor
Re: [Tutor] code review
On 12Jun2014 00:38, Alan Gauld alan.ga...@btinternet.com wrote: On 11/06/14 11:43, Adam Gold wrote: # create snapshot names like the following: 2014-06-10T01-00-01.vm1.img.bz2 for i in vgxenList: DATE = datetime.datetime.now().strftime(%Y-%m-%d + T + %H-%M-%S) Why use addition? You could just insett the literal T... DATE = datetime.datetime.now().strftime(%Y-%m-%dT%H-%M-%S) Also using all caps for the name suggests a constant by convention. vgxenName = /dev/vgxen/ lvName = i Just further to these two snippets, /dev/vgxen/ is just the kind of thing you would treat as a constant (Python doesn't really have constants, just the convention that names in all UPPERCASE represent stable ad hoc values which won't change, and would be constants in some other languages.) So at the top of your script (after imports) you might have: VXGEN_DIR = /dev/vgxen and use it later. Note, no trailing slash in that dir name. In view of this, later where you append to backupList you might go: snap_path = os.path.join(VXGEN_DIR, snapName) backupList.append(snap_name) os.path.join is the usual way to assemble pathnames form path components; it is in principle platform independent. BTW, someone has proably mentioned it, but in case not: names like this_that are the convention for normal variables in Python as opposed to thisThat camelcase names. The main use of camelcase in Python names is class names This That. It doesn't affect your code in any functional way, but others will find you code slightly more readable. These common conventions are outlined in PEP 8: http://legacy.python.org/dev/peps/pep-0008/ Worth a read. It is only strict for stdlib module source code, but adherence is common in most code. Cheers, Cameron Simpson c...@zip.com.au A good catchword can obscure analysis for fifty years. ___ Tutor maillist - Tutor@python.org To unsubscribe or change subscription options: https://mail.python.org/mailman/listinfo/tutor
Re: [Tutor] code review
Post it somewhere on github and I'll try to take a look at it. Lukas On 06/10/2014 05:51 PM, Adam Gold wrote: Hi there. I've been writing a script that is now finished and working (thanks, in part, to some helpful input from this board). What I'd really like to do now is go through it with an 'expert' who can point out ways I may have been able to code more efficiently/effectively. I don't think it would be appropriate to post the whole script here and ask how could I do this better (!) so I was wondering if anyone knows of ways for python noobs to connect with python experts for this sort of exercise. I understand people can be really busy so I'm happy to pay for someone's time if necessary. ___ Tutor maillist - Tutor@python.org To unsubscribe or change subscription options: https://mail.python.org/mailman/listinfo/tutor ___ Tutor maillist - Tutor@python.org To unsubscribe or change subscription options: https://mail.python.org/mailman/listinfo/tutor
Re: [Tutor] code review
On 11/06/14 00:04, Steven D'Aprano wrote: On Tue, Jun 10, 2014 at 04:51:20PM +0100, Adam Gold wrote: Hi there. I've been writing a script that is now finished and working (thanks, in part, to some helpful input from this board). What I'd really like to do now is go through it with an 'expert' who can point out ways I may have been able to code more efficiently/effectively. I don't think it would be appropriate to post the whole script here and ask how could I do this better (!) so I was wondering if anyone knows of ways for python noobs to connect with python experts for this sort of exercise. I understand people can be really busy so I'm happy to pay for someone's time if necessary. How big is the script? A single file, or hundreds of files? Fifty lines of code? A thousand? Thanks for the reply Steven. It's no more than 100 lines at a guess (I'm quite a noob although I think/hope that the brevity of the script may be partially attributable to some choices I made). In simple English, what does it do? Does it require specialized knowledge to understand? It's a backup script, more specifically: - create lv snapshots of pre-selected logical volumes on my server which are running xen VMs - dd and bzip2 (using a pipe) the snapshots to .img.bz2 files for storage on the same server - gpg encrypt the same files and uploads them to s3 - removes the lv snapshots and the .gpg files - deletes files in the S3 directory which are older than X days Is it available somewhere on the Internet? E.g. on Google code, github, sourceforge, your own personal website? Are there confidentiality restrictions on it? The are no restrictions (license or otherwise), no confidentiality issues. The file is not currently available on the internet but I can make it available easily enough on pastebin, as a downloadable file etc. The answer to these questions will influence the type of code review you get. I look forward to hearing further thoughts. ___ Tutor maillist - Tutor@python.org To unsubscribe or change subscription options: https://mail.python.org/mailman/listinfo/tutor
Re: [Tutor] code review
On 11/06/14 00:30, Adam Gold wrote: Thanks for the reply Steven. It's no more than 100 lines at a guess In that case just copy and paste it into a message and send it to the group. Anyone with time available can then take a peek. hth -- Alan G Author of the Learn to Program web site http://www.alan-g.me.uk/ http://www.flickr.com/photos/alangauldphotos ___ Tutor maillist - Tutor@python.org To unsubscribe or change subscription options: https://mail.python.org/mailman/listinfo/tutor
Re: [Tutor] code review
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 11/06/14 08:11, Alan Gauld wrote: On 11/06/14 00:30, Adam Gold wrote: Thanks for the reply Steven. It's no more than 100 lines at a guess In that case just copy and paste it into a message and send it to the group. Anyone with time available can then take a peek. hth One way noobs anywhere can learn is by listening in to other people's conversations - it's called lurking, I believe. So I would say, please do this on the list, and many more people than Adam may benefit. Others can ignore the thread if they wish. Bob - -- Bob Williams System: Linux 3.11.10-11-desktop Distro: openSUSE 13.1 (x86_64) with KDE Development Platform: 4.13.1 Uptime: 06:00am up 3 days 11:36, 3 users, load average: 0.05, 0.03, 0.05 -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.22 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iEYEARECAAYFAlOYFfkACgkQ0Sr7eZJrmU5EUwCgkvjIWEdp1AzodJj6j5fY5yAL wtsAoKFSwk2U4kq5HW5KsmeErH+9fcXI =lJCF -END PGP SIGNATURE- ___ Tutor maillist - Tutor@python.org To unsubscribe or change subscription options: https://mail.python.org/mailman/listinfo/tutor
Re: [Tutor] code review
Thanks for the reply Steven. It's no more than 100 lines at a guess In that case just copy and paste it into a message and send it to the group. Anyone with time available can then take a peek. One way noobs anywhere can learn is by listening in to other people's conversations - it's called lurking, I believe. So I would say, please do this on the list, and many more people than Adam may benefit. Others can ignore the thread if they wish. Bob Oke doke, here it is below. Just for convenience's sake, I'm going to repeat what the basic steps are. It's a backup script for certain xen virtual machines (VM) running on my server. Each VM runs on its own logical volume (as opposed to a file-based loop device). From my own (bitter) experience, the absolutely best way to back up a VM running on a logical volume is to clone it to an image file using dd. I'm aware that a separate discussion could be had around this (on a different mailing list) but, unless someone thinks this is a horribly flawed approach, it may be best to assume this approach is 'fine' so as not to distract from the code review!! Here are the steps: 1) create snapshots of the xen logical volumes using the built in snapshot feature of LVM2 (this way I can backup each logical volume without having to shut down the VM) 2) dd and bzip2 (using a pipe) the snapshots to .img.bz2 files for storage on the same server 3) gpg encrypt the same files and upload them to Amazon s3 4) remove the logical volume snapshots (because they accumulate disk space and I'm doing this daily) and the .gpg files 5) deletes files in the s3 directory which are older than X days As I've mentioned, I'm a real noob, so I'm still mastering some basic stuff. The script works fine for my purposes, I'm keen to understand where it could be improved from a python pov. Finally, yes I could have written this in bash but I prefer python! P.S. I think some of the comments have been wrapped onto more than one line by my email client, I hope this doesn't cause too much inconvenience. #!/usr/bin/python3 ## XEN VIRTUAL MACHINE BACKUP SCRIPT ## ## Copyright (C) 2014 Adam Gold ## ## This program is free software: you can redistribute it and/or modify ## it under the terms of the GNU General Public License as published by ## the Free Software Foundation, either version 3 of the License, or (at ## your option) any later version. ## ## This program is distributed in the hope that it will be useful, but ## WITHOUT ANY WARRANTY; without even the implied warranty of ## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See ## the GNU General Public License for more details. ## ## You should have received a copy of the GNU General Public License ## along with this program. If not see http://gnu.org/licenses/ ## ## Version: 0.4 ## 2014-06-10 import datetime, time, subprocess, shlex, os, gnupg, glob, shutil # logical volumes exist in two different volume groups, vgxen and vg_data # hence two lists of vms vgxenList = ['vm1', 'vm2', 'vm3', 'vm4', 'vm5', 'vm6' ] vg_dataList = ['vm1', 'vm2'] backupList = [ ] snapNameList = [ ] # create snapshot names like the following: 2014-06-10T01-00-01.vm1.img.bz2 for i in vgxenList: DATE = datetime.datetime.now().strftime(%Y-%m-%d + T + %H-%M-%S) vgxenName = /dev/vgxen/ lvName = i origName = vgxenName + lvName snapName= DATE + . + lvName snapNameList.append(snapName) backupList.append(vgxenName + snapName) subprocess.call(['lvcreate', '-s', '-L1G', origName, '-n', snapName]) for h in vg_dataList: DATE = datetime.datetime.now().strftime(%Y-%m-%d + T + %H-%M-%S) vg_dataName = /dev/vg_data/ lvName = h origName = vg_dataName + lvName snapName = DATE + . + lvName snapNameList.append(snapName) backupList.append(vg_dataName + snapName) subprocess.call(['lvcreate', '-s', '-L1G', origName, '-n', snapName]) # backupPath is list of full paths of each snapshot # the string is extacted from backupList using 'join' backupPath = ' '.join(backupList) for j, k in zip(backupList, snapNameList): backupPath = j backupSnapshot = k # run dd and pipe to bz2 file using subprocess module ddIf = shlex.split(dd if=%s bs=4k conv=noerror,notrunc,sync % (backupPath)) compress = pbzip2 filename = /home/files/temp/%s.img.bz2 % (backupSnapshot) p1 = subprocess.Popen(ddIf, stdout=subprocess.PIPE) with p1.stdout as fin, open(filename, w) as fout: p2 = subprocess.Popen(compress, stdin=fin, stdout=fout) ret1 = p1.wait() ret2 = p2.wait() # create list of files to be encrypted with full path names # start with list of unencrypted files cryptDir = '/home/files/temp/' unencrypted = [u for u in os.listdir(cryptDir)] # join absolute path to file names to create new list (list comprehension) cryptDir_unencrypted = [
Re: [Tutor] code review
Ok, not so bad, hoewer there are some parts of the code that could be
done a bit cleaner.
I'll write them below in the response.
Thanks for the reply Steven. It's no more than 100 lines at a
guess
In that case just copy and paste it into a message and send it to
the group. Anyone with time available can then take a peek.
One way noobs anywhere can learn is by listening in to other people's
conversations - it's called lurking, I believe.
So I would say, please do this on the list, and many more people than
Adam may benefit. Others can ignore the thread if they wish.
Bob
Oke doke, here it is below. Just for convenience's sake, I'm going to
repeat what the basic steps are. It's a backup script for certain xen
virtual machines (VM) running on my server. Each VM runs on its own
logical volume (as opposed to a file-based loop device). From my own
(bitter) experience, the absolutely best way to back up a VM running on
a logical volume is to clone it to an image file using dd. I'm aware
that a separate discussion could be had around this (on a different
mailing list) but, unless someone thinks this is a horribly flawed
approach, it may be best to assume this approach is 'fine' so as not to
distract from the code review!!
Here are the steps:
1) create snapshots of the xen logical volumes using the built in
snapshot feature of LVM2 (this way I can backup each logical volume
without having to shut down the VM)
2) dd and bzip2 (using a pipe) the snapshots to .img.bz2 files for
storage on the same server
3) gpg encrypt the same files and upload them to Amazon s3
4) remove the logical volume snapshots (because they accumulate disk
space and I'm doing this daily) and the .gpg files
5) deletes files in the s3 directory which are older than X days
As I've mentioned, I'm a real noob, so I'm still mastering some basic
stuff. The script works fine for my purposes, I'm keen to understand
where it could be improved from a python pov. Finally, yes I could have
written this in bash but I prefer python!
P.S. I think some of the comments have been wrapped onto more than one
line by my email client, I hope this doesn't cause too much inconvenience.
#!/usr/bin/python3
## XEN VIRTUAL MACHINE BACKUP SCRIPT
##
## Copyright (C) 2014 Adam Gold
##
## This program is free software: you can redistribute it and/or modify
## it under the terms of the GNU General Public License as published by
## the Free Software Foundation, either version 3 of the License, or (at
## your option) any later version.
##
## This program is distributed in the hope that it will be useful, but
## WITHOUT ANY WARRANTY; without even the implied warranty of
## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
## the GNU General Public License for more details.
##
## You should have received a copy of the GNU General Public License
## along with this program. If not see http://gnu.org/licenses/
##
## Version: 0.4
## 2014-06-10
import datetime, time, subprocess, shlex, os, gnupg, glob, shutil
imports should be one module per line and alphabetically- more readable:
import datetime
import glob
import gnupg
import os
import shlex
import shutil
import subprocess
import time
# logical volumes exist in two different volume groups, vgxen and vg_data
# hence two lists of vms
vgxenList = ['vm1', 'vm2', 'vm3', 'vm4', 'vm5', 'vm6' ]
vg_dataList = ['vm1', 'vm2']
backupList = [ ]
snapNameList = [ ]
# create snapshot names like the following: 2014-06-10T01-00-01.vm1.img.bz2
for i in vgxenList:
DATE = datetime.datetime.now().strftime(%Y-%m-%d + T + %H-%M-%S)
vgxenName = /dev/vgxen/
lvName = i
origName = vgxenName + lvName
snapName= DATE + . + lvName
snapNameList.append(snapName)
backupList.append(vgxenName + snapName)
subprocess.call(['lvcreate', '-s', '-L1G', origName, '-n', snapName])
for h in vg_dataList:
DATE = datetime.datetime.now().strftime(%Y-%m-%d + T + %H-%M-%S)
vg_dataName = /dev/vg_data/
lvName = h
origName = vg_dataName + lvName
snapName = DATE + . + lvName
snapNameList.append(snapName)
backupList.append(vg_dataName + snapName)
subprocess.call(['lvcreate', '-s', '-L1G', origName, '-n', snapName])
Here in the two loops, you can notice they're almost the same ...
NOTE: I thing you're dealing with file paths, you should use os.path and
os.path.join instead of string conactenation
and you should also check if the file exists first before
calling subprocess ... you can figure out how yourself
Basic DRY (don't repeat yourself) principle - distill their essence into
a function or more:
def snapshot_name(item, name):
:param item: string (one of 'vm1', 'vm2' ..)
:param name: string ('/dev/vgxen/')
isodate = datetime.datetime.now().isoformat() # notice datetime
already has this time format
Re: [Tutor] code review
On 11/06/14 11:43, Adam Gold wrote: # create snapshot names like the following: 2014-06-10T01-00-01.vm1.img.bz2 for i in vgxenList: DATE = datetime.datetime.now().strftime(%Y-%m-%d + T + %H-%M-%S) Why use addition? You could just insett the literal T... DATE = datetime.datetime.now().strftime(%Y-%m-%dT%H-%M-%S) Also using all caps for the name suggests a constant by convention. vgxenName = /dev/vgxen/ lvName = i why not just say for lvname in vxgenlist and avoid the extra assignment? origName = vgxenName + lvName snapName= DATE + . + lvName Again you could have done that in the original strftime() call. snapNameList.append(snapName) backupList.append(vgxenName + snapName) subprocess.call(['lvcreate', '-s', '-L1G', origName, '-n', snapName]) for h in vg_dataList: DATE = datetime.datetime.now().strftime(%Y-%m-%d + T + %H-%M-%S) see above vg_dataName = /dev/vg_data/ lvName = h again, why not just use the meaningful name in the for loop? origName = vg_dataName + lvName snapName = DATE + . + lvName snapNameList.append(snapName) backupList.append(vg_dataName + snapName) subprocess.call(['lvcreate', '-s', '-L1G', origName, '-n', snapName]) In fact these loops are so similar you should probably make a function with a couple of parameters and call it from both loops. The only things different are the list you loop over and the path. Make those params and you get something like this (untested): def create_volumes(volList, path): for name in volList: dt = datetime.datetime.now().strftime(%Y-%m-%dT%H-%M-%S.+name) dataName = path origName = dataName + name snapName = DATE + . + name snapNameList.append(snapName) backupList.append(dataName + snapName) subprocess.call(['lvcreate', '-s', '-L1G', origName, '-n', snapName]) # backupPath is list of full paths of each snapshot # the string is extacted from backupList using 'join' backupPath = ' '.join(backupList) for j, k in zip(backupList, snapNameList): backupPath = j backupSnapshot = k # run dd and pipe to bz2 file using subprocess module ddIf = shlex.split(dd if=%s bs=4k conv=noerror,notrunc,sync % (backupPath)) compress = pbzip2 filename = /home/files/temp/%s.img.bz2 % (backupSnapshot) p1 = subprocess.Popen(ddIf, stdout=subprocess.PIPE) with p1.stdout as fin, open(filename, w) as fout: p2 = subprocess.Popen(compress, stdin=fin, stdout=fout) Take note of the warning in the subprocess documentation about using stdin/stdout directly. You should ideally access them via Popen.communicate(). ret1 = p1.wait() ret2 = p2.wait() I may just be missing it but I don't see you do anything with these ret1,ret2 variables? # create list of files to be encrypted with full path names # start with list of unencrypted files cryptDir = '/home/files/temp/' unencrypted = [u for u in os.listdir(cryptDir)] This is a redundant comprehension. listdir() returns the list for you. Any time you see foo = [item for item in a_list] it probably should just be: foo = a_list Or if a_list is really an iterator it could be foo = list(an_iter) Comprehensions are good when you have a filter condition on the end or if you are returning a function/expression involving item. # join absolute path to file names to create new list (list comprehension) cryptDir_unencrypted = [ os.path.join(cryptDir, s) for s in unencrypted ] Like that one for example... # encrypt files for G in cryptDir_unencrypted: You seem to like single letter loop variables. Its better to use something more meaningful. Say: for filename in cryptDir_unencrypted: gpg = gnupg.GPG(gnupghome='/root/.gnupg') phrase = passphrase # HORRIBLE SECURITY, I KNOW! The script is running as a cronjob so I can't interactively enter the passphrase. Suggestions are welcome. Use a config file that you can edit when needed. Read the passwords from that. It could even be encrypted... cipher = AES256 with open(G, 'rb') as f: status = gpg.encrypt_file(f, None, armor=False, passphrase=phrase, symmetric=cipher.upper(), output=G + '.gpg') # move unencypted files out of temp directory for data in glob.glob(cryptDir + '*.bz2'): You should really use os.join() instead of string addition... shutil.move(data,'/home/files/') # delete snapshots for r in snapNameList: removeSnapshots1 = 'lvremove -f ' + vgxenName + r subprocess.call(shlex.split(removeSnapshots1)) removeSnapshots2 = 'lvremove -f ' + vg_dataName + r subprocess.call(shlex.split(removeSnapshots2)) # create list of file names to be uploaded (list comprehension) uploads = [y for y in os.listdir(cryptDir)] see previous comment # join absolute path to file names to create new list (list comprehension) cryptDir_uploads = [ os.path.join(cryptDir, t) for t in uploads ] #
[Tutor] code review
Hi there. I've been writing a script that is now finished and working (thanks, in part, to some helpful input from this board). What I'd really like to do now is go through it with an 'expert' who can point out ways I may have been able to code more efficiently/effectively. I don't think it would be appropriate to post the whole script here and ask how could I do this better (!) so I was wondering if anyone knows of ways for python noobs to connect with python experts for this sort of exercise. I understand people can be really busy so I'm happy to pay for someone's time if necessary. ___ Tutor maillist - Tutor@python.org To unsubscribe or change subscription options: https://mail.python.org/mailman/listinfo/tutor
Re: [Tutor] code review
On Tue, Jun 10, 2014 at 04:51:20PM +0100, Adam Gold wrote: Hi there. I've been writing a script that is now finished and working (thanks, in part, to some helpful input from this board). What I'd really like to do now is go through it with an 'expert' who can point out ways I may have been able to code more efficiently/effectively. I don't think it would be appropriate to post the whole script here and ask how could I do this better (!) so I was wondering if anyone knows of ways for python noobs to connect with python experts for this sort of exercise. I understand people can be really busy so I'm happy to pay for someone's time if necessary. How big is the script? A single file, or hundreds of files? Fifty lines of code? A thousand? In simple English, what does it do? Does it require specialized knowledge to understand? Is it available somewhere on the Internet? E.g. on Google code, github, sourceforge, your own personal website? Are there confidentiality restrictions on it? The answer to these questions will influence the type of code review you get. -- Steven ___ Tutor maillist - Tutor@python.org To unsubscribe or change subscription options: https://mail.python.org/mailman/listinfo/tutor
Re: [Tutor] Code Review?
On Mon, Aug 6, 2012 at 6:27 PM, Brian Carpio bcar...@thetek.net wrote: Thanks to everyone who has replied! This is some good information for me to go learn with!. I greatly appreciate it. When you refactor your code, let us know. I, for one, would like to see it. ___ Tutor maillist - Tutor@python.org To unsubscribe or change subscription options: http://mail.python.org/mailman/listinfo/tutor
[Tutor] Code Review?
Hi, Hopefully I am allowed to ask this here. I am pretty new to python I've only been writing code for about 6 months now strictly for system administration purposes; however I have now decided to write something real that others might benefit from but I am looking for someone to take a look at my code and give me an idea on some areas where I need improvement. I certainly don't expect anyone to rewrite my code but if someone is willing to give it a quick look and tell me things like focus more on XX or you really need to learn YY because its obvious you don't have a clue (lol) those types of comments would be more then welcome. I've ran my code through pylint and am getting a score of 7-9 with all of my scripts, but a program can only tell you so much having a real person point out some of my shortcomings would be amazing. Sometimes you don't know you don't know something until someone tells you Hey you don't know XX or YY go learn it. https://github.com/bcarpio/mongodb-enc Thanks, Brian ___ Tutor maillist - Tutor@python.org To unsubscribe or change subscription options: http://mail.python.org/mailman/listinfo/tutor
Re: [Tutor] Code Review?
Another practice is make __license__ = GPL license... and __author__ = Brian Carpio x...@xxx.com I do so. On 6 August 2012 19:08, Ramchandra Apte maniandra...@gmail.com wrote: In scripts/add_node.py GPL Licene should be GPL License On 6 August 2012 18:59, Brian Carpio bcar...@thetek.net wrote: Hi, Hopefully I am allowed to ask this here. I am pretty new to python I've only been writing code for about 6 months now strictly for system administration purposes; however I have now decided to write something real that others might benefit from but I am looking for someone to take a look at my code and give me an idea on some areas where I need improvement. I certainly don't expect anyone to rewrite my code but if someone is willing to give it a quick look and tell me things like focus more on XX or you really need to learn YY because its obvious you don't have a clue (lol) those types of comments would be more then welcome. I've ran my code through pylint and am getting a score of 7-9 with all of my scripts, but a program can only tell you so much having a real person point out some of my shortcomings would be amazing. Sometimes you don't know you don't know something until someone tells you Hey you don't know XX or YY go learn it. https://github.com/bcarpio/mongodb-enc Thanks, Brian ___ Tutor maillist - Tutor@python.org To unsubscribe or change subscription options: http://mail.python.org/mailman/listinfo/tutor ___ Tutor maillist - Tutor@python.org To unsubscribe or change subscription options: http://mail.python.org/mailman/listinfo/tutor
Re: [Tutor] Code Review?
In scripts/add_node.py GPL Licene should be GPL License On 6 August 2012 18:59, Brian Carpio bcar...@thetek.net wrote: Hi, Hopefully I am allowed to ask this here. I am pretty new to python I've only been writing code for about 6 months now strictly for system administration purposes; however I have now decided to write something real that others might benefit from but I am looking for someone to take a look at my code and give me an idea on some areas where I need improvement. I certainly don't expect anyone to rewrite my code but if someone is willing to give it a quick look and tell me things like focus more on XX or you really need to learn YY because its obvious you don't have a clue (lol) those types of comments would be more then welcome. I've ran my code through pylint and am getting a score of 7-9 with all of my scripts, but a program can only tell you so much having a real person point out some of my shortcomings would be amazing. Sometimes you don't know you don't know something until someone tells you Hey you don't know XX or YY go learn it. https://github.com/bcarpio/mongodb-enc Thanks, Brian ___ Tutor maillist - Tutor@python.org To unsubscribe or change subscription options: http://mail.python.org/mailman/listinfo/tutor ___ Tutor maillist - Tutor@python.org To unsubscribe or change subscription options: http://mail.python.org/mailman/listinfo/tutor
Re: [Tutor] Code Review?
On Mon, Aug 6, 2012 at 9:39 AM, Ramchandra Apte maniandra...@gmail.comwrote: Another practice is make __license__ = GPL license... and __author__ = Brian Carpio x...@xxx.com I do so. On 6 August 2012 19:08, Ramchandra Apte maniandra...@gmail.com wrote: In scripts/add_node.py GPL Licene should be GPL License Brian, I glanced at your code, and I saw a lot of duplication between the scripts. Is there a reason for this? -Tino ___ Tutor maillist - Tutor@python.org To unsubscribe or change subscription options: http://mail.python.org/mailman/listinfo/tutor
Re: [Tutor] Code Review?
On 06/08/2012 14:38, Ramchandra Apte wrote: [top posting fixed] On 6 August 2012 18:59, Brian Carpio bcar...@thetek.net wrote: Hi, Hopefully I am allowed to ask this here. I am pretty new to python I've only been writing code for about 6 months now strictly for system administration purposes; however I have now decided to write something real that others might benefit from but I am looking for someone to take a look at my code and give me an idea on some areas where I need improvement. I certainly don't expect anyone to rewrite my code but if someone is willing to give it a quick look and tell me things like focus more on XX or you really need to learn YY because its obvious you don't have a clue (lol) those types of comments would be more then welcome. I've ran my code through pylint and am getting a score of 7-9 with all of my scripts, but a program can only tell you so much having a real person point out some of my shortcomings would be amazing. Sometimes you don't know you don't know something until someone tells you Hey you don't know XX or YY go learn it. https://github.com/bcarpio/mongodb-enc Thanks, Brian ___ Tutor maillist - Tutor@python.org To unsubscribe or change subscription options: http://mail.python.org/mailman/listinfo/tutor ___ Tutor maillist - Tutor@python.org To unsubscribe or change subscription options: http://mail.python.org/mailman/listinfo/tutor In scripts/add_node.py GPL Licene should be GPL License Please don't top post, it makes it very difficult to follow threads. Thanks. -- Cheers. Mark Lawrence. ___ Tutor maillist - Tutor@python.org To unsubscribe or change subscription options: http://mail.python.org/mailman/listinfo/tutor
Re: [Tutor] Code Review?
Brian Carpio wrote: Hi, Hopefully I am allowed to ask this here. I am pretty new to python I've only been writing code for about 6 months now strictly for system administration purposes; however I have now decided to write something real that others might benefit from but I am looking for someone to take a look at my code and give me an idea on some areas where I need improvement. I certainly don't expect anyone to rewrite my code but if someone is willing to give it a quick look and tell me things like focus more on XX or you really need to learn YY because its obvious you don't have a clue (lol) those types of comments would be more then welcome. I've ran my code through pylint and am getting a score of 7-9 with all of my scripts, but a program can only tell you so much having a real person point out some of my shortcomings would be amazing. Sometimes you don't know you don't know something until someone tells you Hey you don't know XX or YY go learn it. https://github.com/bcarpio/mongodb-enc [I took a look at mongodb_node_classifier.py] Random remarks: - pylint probably told you: use self-explanatory names rather than i, d, or n. - Split your code in smaller dedicated functions. This simplifies writing unittests, allows reuse and keeps control flow manageable as your script grows. - Add a comment that explains what the script does; repeat for each function. - Use main() to read the configuration/commandline and put the real meat into a dedicated function. - Prefer raising exceptions over sys.exit() in the code that does the real work (main() may convert these into sys.exit). Here's a sketch for a revised main(): def main(): node_name = sys.argv[1] filename = ... conn_spec = read_config(filename) connection = open_connection(con_spec) try: process(connection, node_name) except InheritanceError as err: sys.exit(str(err)) (You should choose a name that is more descriptive than process()) - Use 'expr', not 'expr == True' in boolean contexts - Show that you know None is a singleton and prefer 'expr is None' over 'expr == None' - I found the while loop hard to understand. I have a hunch that it can be simplified. ___ Tutor maillist - Tutor@python.org To unsubscribe or change subscription options: http://mail.python.org/mailman/listinfo/tutor
Re: [Tutor] Code Review?
Thanks to everyone who has replied! This is some good information for me to go learn with!. I greatly appreciate it. Brian On Mon, Aug 6, 2012 at 9:21 AM, Peter Otten __pete...@web.de wrote: Brian Carpio wrote: Hi, Hopefully I am allowed to ask this here. I am pretty new to python I've only been writing code for about 6 months now strictly for system administration purposes; however I have now decided to write something real that others might benefit from but I am looking for someone to take a look at my code and give me an idea on some areas where I need improvement. I certainly don't expect anyone to rewrite my code but if someone is willing to give it a quick look and tell me things like focus more on XX or you really need to learn YY because its obvious you don't have a clue (lol) those types of comments would be more then welcome. I've ran my code through pylint and am getting a score of 7-9 with all of my scripts, but a program can only tell you so much having a real person point out some of my shortcomings would be amazing. Sometimes you don't know you don't know something until someone tells you Hey you don't know XX or YY go learn it. https://github.com/bcarpio/mongodb-enc [I took a look at mongodb_node_classifier.py] Random remarks: - pylint probably told you: use self-explanatory names rather than i, d, or n. - Split your code in smaller dedicated functions. This simplifies writing unittests, allows reuse and keeps control flow manageable as your script grows. - Add a comment that explains what the script does; repeat for each function. - Use main() to read the configuration/commandline and put the real meat into a dedicated function. - Prefer raising exceptions over sys.exit() in the code that does the real work (main() may convert these into sys.exit). Here's a sketch for a revised main(): def main(): node_name = sys.argv[1] filename = ... conn_spec = read_config(filename) connection = open_connection(con_spec) try: process(connection, node_name) except InheritanceError as err: sys.exit(str(err)) (You should choose a name that is more descriptive than process()) - Use 'expr', not 'expr == True' in boolean contexts - Show that you know None is a singleton and prefer 'expr is None' over 'expr == None' - I found the while loop hard to understand. I have a hunch that it can be simplified. ___ Tutor maillist - Tutor@python.org To unsubscribe or change subscription options: http://mail.python.org/mailman/listinfo/tutor ___ Tutor maillist - Tutor@python.org To unsubscribe or change subscription options: http://mail.python.org/mailman/listinfo/tutor
Re: [Tutor] Code review, plase
On Wed, Sep 8, 2010 at 12:20 AM, Steven D'Aprano st...@pearwood.info wrote:
On Wed, 8 Sep 2010 06:39:27 am Alex wrote:
Hi all.
Could someone review my code? It's the first time I develop a
reusable module and I would like to have some feedback.
If you think it's good enough I will package it for pypi.
I put the code on pastebin: http://pastebin.com/Tz367gAM
Let's start with some little things... you talk about the Unix CronTab
command, but there's no such thing, at least on Linux:
[st...@sylar ~]$ CronTab
bash: CronTab: command not found
Absolutely right I'll update the docstring
In the _remove_crontab method, you say *All* the information contained
in the crontab file are permanently lost, but that's grammatically
incorrect -- information is a mass noun, i.e. neither singular nor
plural. So in standard English, you would say all the information in
the file is permanently lost, not are lost, in the same way that you
would say the sugar is on the table or the grass is green.
Argh! I should go back to grammar school :-) English is not my first langage
ct1 = micron.CronTab()
ct2 = micron.CronTab()
ct1.add_job('daily', 'echo BOOM!')
ct2.add_job('daily', 'echo No BOOM today')
Do they fight? What happens?
They will not be duplicated because the class will generate a unique
job_id incrementing the last id used. Since the module is just a
wrapper for the crontab command any other clashing will be managed by
the cron daemon. I will add an example to document this behaviour.
Thanks for your helpful comments.
Alex
___
Tutor maillist - Tutor@python.org
To unsubscribe or change subscription options:
http://mail.python.org/mailman/listinfo/tutor
[Tutor] Code review, plase
Hi all. Could someone review my code? It's the first time I develop a reusable module and I would like to have some feedback. If you think it's good enough I will package it for pypi. I put the code on pastebin: http://pastebin.com/Tz367gAM Thanks in advance. Alex ___ Tutor maillist - Tutor@python.org To unsubscribe or change subscription options: http://mail.python.org/mailman/listinfo/tutor
Re: [Tutor] Code review, plase
On Tue, Sep 7, 2010 at 3:39 PM, Alex metallourla...@gmail.com wrote: Hi all. Could someone review my code? It's the first time I develop a reusable module and I would like to have some feedback. If you think it's good enough I will package it for pypi. I put the code on pastebin: http://pastebin.com/Tz367gAM Thanks in advance. Alex I mostly just glanced through it and didn't notice any gross errors. Your comments should be standardized - I think I got them all here: http://pastebin.com/BZSTRVWd Also, I changed line 83 to say what I think you meant to say. I could be wrong, but I don't think something like Job 342 exist is what you meant. HTH, -Wayne ___ Tutor maillist - Tutor@python.org To unsubscribe or change subscription options: http://mail.python.org/mailman/listinfo/tutor
Re: [Tutor] Code review, plase
On Wed, 8 Sep 2010 06:39:27 am Alex wrote:
Hi all.
Could someone review my code? It's the first time I develop a
reusable module and I would like to have some feedback.
If you think it's good enough I will package it for pypi.
I put the code on pastebin: http://pastebin.com/Tz367gAM
Let's start with some little things... you talk about the Unix CronTab
command, but there's no such thing, at least on Linux:
[st...@sylar ~]$ CronTab
bash: CronTab: command not found
It would be very unusual for a Unix command to be written in CamelCase.
The command is actually crontab.
In the _remove_crontab method, you say *All* the information contained
in the crontab file are permanently lost, but that's grammatically
incorrect -- information is a mass noun, i.e. neither singular nor
plural. So in standard English, you would say all the information in
the file is permanently lost, not are lost, in the same way that you
would say the sugar is on the table or the grass is green.
http://en.wikipedia.org/wiki/Mass_noun
You overuse leading-underscore method names. Use them only for private
methods. You use them for public methods, and even give an example of
how to remove the entire file:
#Remove the entire crontab file
crontab._remove_crontab()
When you're telling people to use this command, that's a good sign that
it is public not private!
In PRESETS, what's mothly mean? *wink*
What happens if I do this?
ct1 = micron.CronTab()
ct2 = micron.CronTab()
ct1.add_job('daily', 'echo BOOM!')
ct2.add_job('daily', 'echo No BOOM today')
Do they fight? What happens?
--
Steven D'Aprano
___
Tutor maillist - Tutor@python.org
To unsubscribe or change subscription options:
http://mail.python.org/mailman/listinfo/tutor
[Tutor] code review request
Hoping to get some of you guru types to look over the start of a tool I
am working on in python.
A working version of the script is at
https://mikaru.homeunix.org/py-bin/memberlist.py
The site only allows https because I got sick of all the hacked windoze
boxes trying to execute crap that I didn't have, so port 80(http) is
blocked on my firewall.
This lets you add users, divisions (groups) and put the users in
divisions(groups). and list the users out by group. I haven't figure
out yet how to authenticate the users from the database (postgresql) so
any pointers there would be helpful. When a user is added, the password
is encrypted in the database using postgresql's encrypt() function so
that it would be possible to use another application to access the
data. Any pointers or advise on where improvments could be made would
be welcome.
#!/usr/bin/python
print 'Content-type: text/html\n'
import psycopg
import cgitb
import cgi
import sys
cgitb.enable()
def quote(string):
if string:
return string.replace(', \\')
else:
return string
form = cgi.FieldStorage()
conn = psycopg.connect('dbname=XXX user=xxx password=x')
curs = conn.cursor()
div_name = quote(form.getvalue('div_name'))
div_director = quote(form.getvalue('div_director'))
div_email = quote(form.getvalue('div_email'))
if not (div_name and div_director and div_email):
print 'ALL FIELDS MUST BE COMPLETED'
sys.exit()
query = INSERT INTO divisions(div_name, div_director, div_email) VALUES
('%s', '%s', '%s') % (div_name, div_director, div_email)
curs.execute(query)
conn.commit()
conn.close()
print
html
head
titleDivision added/title
/head
body
h1Division created successfully/h1
hr /
a href='memberlist.py'Back to the main page/a
/body
/html
#!/usr/bin/python
print 'Content-type: text/html\n'
import psycopg
import cgitb
import cgi
import sys
cgitb.enable()
def quote(string):
if string:
return string.replace(', \\')
else:
return string
form = cgi.FieldStorage()
conn = psycopg.connect('dbname= user=x password=x')
curs = conn.cursor()
name = quote(form.getvalue('name'))
address = quote(form.getvalue('address'))
email = quote(form.getvalue('email'))
password = quote(form.getvalue('password'))
username = quote(form.getvalue('username'))
div_id = quote(form.getvalue('division'))
if not (name and username and password):
print 'Please supply name, username, and password'
sys.exit()
query = INSERT INTO members(name, address, email, password, username,
div_id) VALUES ('%s', '%s', '%s', encrypt('%s', \'f00zball\', \'aes\'), '%s',
'%i') % (name, address, email, password, username, int(div_id))
curs.execute(query)
conn.commit()
conn.close()
print
html
head
titleUser added/title
/head
body
h1User created successfully/h1
hr /
a href='memberlist.py'Back to the main page/a
/body
/html
#!/usr/bin/python
from mod_python import apache
import cgitb; cgitb.enable()
import psycopg
conn = psycopg.connect('dbname= user= password=x')
curs = conn.cursor()
print 'Content-type: text/html\n'
print
html
head
titleMember Management/title
/head
body
h1User List/h1
curs.execute('SELECT * FROM divisions')
rows = curs.dictfetchall()
toplevel = []
children = {}
for row in rows:
division = row['div_id']
print 'pa href=viewdiv.py?div_id=%(div_id)i%(div_name)s/a/p' % row
def format(row):
print 'pa href=viewdiv.py?div_id=%(div_id)i%(div_name)s/a/p' % row
try: kids = children[row['div_id']]
except KeyError: pass
else:
print 'blockquote'
for kid in kids:
format(kid)
print '/blockquote'
print 'p'
for row in toplevel:
format(row)
print
/p
hr /
pa href=newuser.pyCreate User/a | a href=new_div.pyAdd
Division/A/p
/body
/html
#!/usr/bin/python
from mod_python import apache
import cgitb; cgitb.enable()
import psycopg
conn = psycopg.connect('dbname=x user= password=x')
curs = conn.cursor()
print 'Content-type: text/html\n'
print
html
head
titleMember Management/title
/head
body
h1User List/h1
curs.execute('SELECT * FROM members')
rows = curs.dictfetchall()
toplevel = []
children = {}
for row in rows:
parent_id = row['div_id']
if parent_id is None:
toplevel.append(row)
else:
children.setdefault(parent_id,[]).append(row)
def format(row):
print 'pa href=viewuser.py?mem_id=%(mem_id)i%(name)s/a/p' % row
try: kids = children[row['mem_id']]
except KeyError: pass
else:
print 'blockquote'
for kid in kids:
format(kid)
print '/blockquote'
print 'p'
for row in toplevel:
format(row)
print
/p
hr /
pa href=newuser.pyCreate User/a | a href=new_div.pyAdd
Division/A | A HREF=div_list.pyList Divisions/A/p
/body
/html
#!/usr/bin/python
print 'Content-type: text/html\n'
import cgitb; cgitb.enable()
import
Re: [Tutor] code review please
Eakin, W said unto the world upon 27/12/05 09:59 AM:
Hello,
Although I've been coding in PHP and ASP and JavaScript for a couple of
years now, I'm relatively new to Python. For learning exercises, I'm writing
small Python programs that do limited things, but hopefully do them well.
The following program takes a text file, reads through it, and any word
longer than four characters will have the internal letters scrambled, but
the first and last letters of the word will remain unchanged. Here's what
happened when I ran the program on a file called example.txt.
Before:
This is a sample of text that has been scrambled, before and after.
After:
Tihs is a sapmle of txet taht has been sblrmcead, broefe and aetfr.
The code follows, so any comments and/or suggestions as to what I did right
or wrong, or what could be done better will be appreciated.
thanks,
William
Hi William,
I coded up an approach; no guarantees that it is anywhere near optimal :-)
I didn't bother with the file I/O portions. Also, it respects internal
punctuation in compound-words and the like. It does not respect extra
white-space in the sense that A cat and A cat produce identical
output.
Best,
Brian vdB
import random
from string import punctuation
tstring = '''
This is my test string for the scramble_words function. It contains lots
of punctuation marks like ')', and '?'--well, not lots: instead, enough!
Here's what happens when one occurs mid-word: punct#uation.'''
def scramble_words(a_string):
'''returns a_string with all internal substrings of words randomized
The scramble_words function respects punctuation in that a word is a
maximal string with neither whitespace nor characters from
punctuation.
Each word is scrambled in the sense that the characters excepting the
first and last character are randomized.
'''
output = []
for sequence in a_string.split():
chunks = punctuation_split(sequence)
# appending the joined chunks prevents spurious spaces
# around punctuation marks
output.append(''.join([_scramble_word(x) for x in chunks]))
output = ' '.join(output)
return output
def punctuation_split(sequence):
'''returns list of character sequences separating punctuation
characters'''
for mark in punctuation:
sequence = sequence.replace(mark, ' %s ' %mark)
return sequence.split(' ')
def _scramble_word(word):
'''returns word with its internal substring randomized'''
if len(word) 4:
return word
middle = list(word[1:-1])
random.shuffle(middle)
return ''.join((word[0], ''.join(middle), word[-1]))
a = scramble_words(tstring)
print a
___
Tutor maillist - Tutor@python.org
http://mail.python.org/mailman/listinfo/tutor
Re: [Tutor] code review please
Brian van den Broek wrote:
def punctuation_split(sequence):
'''returns list of character sequences separating punctuation
characters'''
for mark in punctuation:
sequence = sequence.replace(mark, ' %s ' %mark)
return sequence.split(' ')
You should look at re.split().
Kent
___
Tutor maillist - Tutor@python.org
http://mail.python.org/mailman/listinfo/tutor
Re: [Tutor] code review please
Danny Yoo wrote: Similarly, the try/except for IndexError seems too pervasive: rather than wrap it around the whole program, we may want to limit its extent to just around the sys.argv access. Otherwise, any other IndexError has the potential of being misattributed. Much of the program does indexing of some sort, so that's why this concerns me. Alternatively, doing the explicit length check: if not sys.argv[1:]: print some kind of usage message raise SystemExit might be sufficient. Unless we really want to hide errors from the user, I'd avoid the exception handlers altogether: if bad things happen, the default exception handler gives a fairly good stack trace that aids debugging. But if we do want to handle the exceptions manually, we should try to make sure that useful information is preserved in the error messages: it's a terrible thing to get an error message that says Something bad happened. when we can do much better. *grin* I agree with Danny that in this case there is no need to catch the exceptions - just let the default exception handler do its thing. If you *do* want to handle the exceptions yourself, a good principle is to put the try / except around the least amount of code possible - just the lines that may generate the expected exception. This prevents the except clause from hiding unexpected exceptions. An easy way to do this is to use try / except / else. The else clause is only executed if no exception was caught by the except clause. In your case you could write try: fname = sys.argv[1] except IndexError: # report the error as you like else: # normal processing goes here Kent ___ Tutor maillist - Tutor@python.org http://mail.python.org/mailman/listinfo/tutor
Re: [Tutor] code review please
Kent Johnson wrote: BTW thinking of writing this as a loop brings to mind some problems - what will your program do with 'words' such as 555-1212 or Ha! ? Hmm, on reflection I don't thing Ha! will be a problem, but a 'word' with no letters will cause an IndexError. Your test for 4 letters is really misplaced. What you want to find is words that have only two letters to scramble. I would put a test right where you call random.shuffle() to see if tempWord is longer than 2. In fact you might put the whole test and shuffle bit in a separate function. Kent ___ Tutor maillist - Tutor@python.org http://mail.python.org/mailman/listinfo/tutor
Re: [Tutor] code review please
Kent Johnson said unto the world upon 28/12/05 07:06 AM:
Brian van den Broek wrote:
def punctuation_split(sequence):
'''returns list of character sequences separating punctuation
characters'''
for mark in punctuation:
sequence = sequence.replace(mark, ' %s ' %mark)
return sequence.split(' ')
You should look at re.split().
Kent
What, and have *2* problems? :-)
(But seriously, thanks for the pointer. As I was coding, I thought
there must have been a better way drawing off of the library. See my
other post for why I couldn't find it.)
Best,
Brian vdB
___
Tutor maillist - Tutor@python.org
http://mail.python.org/mailman/listinfo/tutor
[Tutor] code review please
Hello,
Although I've been coding in PHP and ASP and
_javascript_ for a couple of years now, I'm relatively new to Python. For
learning exercises, I'm writing small Python programs that do limited
things, but hopefully do them well.
The following program takes a text file, reads through it, and any word
longer than four characters will have the internal letters scrambled,
but the first and last letters of the word will remain unchanged.
Here's what happened when I ran the program on a file called
example.txt.
Before:
This is a sample of text that has been scrambled, before and after.
After:
Tihs is a sapmle of txet taht has been sblrmcead, broefe and aetfr.
The code follows, so any comments and/or suggestions as to what I did
right or wrong, or what could be done better will be appreciated.
thanks,
William
#!/usr/bin/env python
#filename: wScramble.py
#filelocation: /home/william/programming/code/python
#filedate: 12/25/2005
import sys
import random
def fileScramble(fileName):
newFile = file('scrambled.txt', 'w')
newRow = ''
for line in fileName:
newRow = ''
tempList = line.split(' ')
for word in tempList:
newRow = newRow + ' ' + wordScramble(word)
newRow = newRow + '\n'
newFile.write(newRow)
newFile.close()
def wordScramble(word):
punctuation = ['.', ',', ':', ';', '(', ')']
if len(word) 4:
return word
elif len(word) == 4 and word[-1] in punctuation or word[0] in punctuation:
return word
elif len(word) == 4:
word = word[0] + word[2] + word[1] + word[3]
return word
else:
(fCut, fLetter) = getLetter(word, 0, 'forward')
(lCut, lLetter) = getLetter(word, -1, 'back')
tempWord = list(word)
del tempWord[:fCut + 1]
del tempWord[lCut:]
random.shuffle(tempWord)
middleString = .join(tempWord)
scrambledWord = fLetter + middleString + lLetter
return scrambledWord
def getLetter(string, number, direction):
if direction == 'forward':
increment = 1
else:
increment = -1
if string[number].isalpha() == True:
if direction == 'forward':
return (number, string[:number + 1])
else:
return (number, string[number:])
elif string[number].isalpha() == False:
return getLetter(string, number + increment, direction)
if __name__ == __main__:
try:
if sys.argv[1].isspace() == True:
print No file
was given to the program to process.\n-- Program
quitting --\n
else:
try:
f = open(sys.argv[1])
fileScramble(f)
f.close()
except IOError:
print That file does not exist, or you do not have permission to
access it.\n-- Program quitting --\n
except IndexError:
print No file was given to the
program to process.\n-- Program quitting
--\n
___
Tutor maillist - Tutor@python.org
http://mail.python.org/mailman/listinfo/tutor
Re: [Tutor] code review please
Hi William,
Here are some constructive comments on the program; if you have questions
on any of it, please feel free to ask.
def fileScramble(fileName):
We may want to rename fileName to something else, as it isn't being
treated as a filename, but more like a file-like object. Good names help
to clarify programs. I had expected the input here to be a string, and to
see it be a file-like object was a little disorienting at first.
[within fileScramble() ...]
newFile = file('scrambled.txt', 'w')
newRow = ''
for line in fileName:
It's possible that the lines here still have their trailing newlines: you
might want to rstrip() them out before further processing.
def wordScramble(word):
punctuation = ['.', ',', ':', ';', '(', ')']
if len(word) 4:
return word
elif len(word) == 4 and word[-1] in punctuation or word[0] in
punctuation:
The complexity of the elif is a bit high. Do you mean:
((len(word) == 4 and word[-1] in punctuation) or
word[0] in punctuation)
or do you mean:
len(word) == 4 and (word[-1] in punctuation or
word[0] in punctuation)
Explicit parenthesization is a good idea here, since there are two
possible ways of parsing the expression. It's unambiguous to the computer
of course, but since it's ambiguous for humans, let's make it explicit
what we mean.
def getLetter(string, number, direction):
if direction == 'forward':
increment = 1
else:
increment = -1
if string[number].isalpha() == True:
The above if statement can be simplified to:
if string[number].isalpha(): ...
because comparing True to True is a bit redundant.
In fact, since either string[number].isalpha() is True or it isn't, this
allows us simplify the block:
if string[number].isalpha() == True:
...
elif string[number].isalpha() == False:
...
so that we use a simpler if/else:
if string[number].isalpha():
...
else:
...
Looking into the __main__, I see:
try:
f = open(sys.argv[1])
fileScramble(f)
f.close()
except IOError:
print That file does not exist, or you do not have
In the exception handler, we may want to also show the message of the
exception too, just in case another kind of IOError has occurred.
According to:
http://www.python.org/doc/lib/module-exceptions.html
we can grab at the 'errno' and 'strerror' attributes of the exception
object to display more detailed information --- I'd recommend including
those in the error output, since that's useful debugging information.
Similarly, the try/except for IndexError seems too pervasive: rather than
wrap it around the whole program, we may want to limit its extent to just
around the sys.argv access. Otherwise, any other IndexError has the
potential of being misattributed. Much of the program does indexing of
some sort, so that's why this concerns me.
Alternatively, doing the explicit length check:
if not sys.argv[1:]:
print some kind of usage message
raise SystemExit
might be sufficient.
Unless we really want to hide errors from the user, I'd avoid the
exception handlers altogether: if bad things happen, the default exception
handler gives a fairly good stack trace that aids debugging.
But if we do want to handle the exceptions manually, we should try to make
sure that useful information is preserved in the error messages: it's a
terrible thing to get an error message that says Something bad happened.
when we can do much better. *grin*
I hope this critique helps!
___
Tutor maillist - Tutor@python.org
http://mail.python.org/mailman/listinfo/tutor
Re: [Tutor] code review please
Eakin, W wrote:
The code follows, so any comments and/or suggestions as to what I did
right or wrong, or what could be done better will be appreciated.
def fileScramble(fileName):
newFile = file('scrambled.txt', 'w')
newRow = ''
for line in fileName:
newRow = ''
tempList = line.split(' ')
for word in tempList:
newRow = newRow + ' ' + wordScramble(word)
newRow = newRow + '\n'
newFile.write(newRow)
This seem pretty verbose to me. Using tempList doesn't IMO add anything,
it might as well be
for word in line.split(' '):
or
for word in line.split():
since you probably want to split on tabs also and this will strip the
trailing newlines as a bonus.
I usually prefer a list comprehension to an accumulation loop when
possible so I would actually write it as
newWords = [ wordScramble(word) for word in line.split() ]
newRow = ' '.join(newWords) + '\n'
or even
newRow = ' '.join(wordScramble(word) for word in line.split()) + '\n'
though that might be a little too terse for easy comprehension.
newFile.close()
def wordScramble(word):
punctuation = ['.', ',', ':', ';', '(', ')']
if len(word) 4:
return word
elif len(word) == 4 and word[-1] in punctuation or word[0] in
punctuation:
return word
elif len(word) == 4:
word = word[0] + word[2] + word[1] + word[3]
return word
Rather than repeating the test for len(word) == 4, I would write
elif len(word) == 4:
if word[-1] in punctuation or word[0] in punctuation:
return word
else:
word = word[0] + word[2] + word[1] + word[3]
return word
This also breaks up the long conditional that Danny complained about.
else:
(fCut, fLetter) = getLetter(word, 0, 'forward')
(lCut, lLetter) = getLetter(word, -1, 'back')
tempWord = list(word)
del tempWord[:fCut + 1]
del tempWord[lCut:]
I think this is the same as
tempWord = list(word[fCut+1:lCut])
random.shuffle(tempWord)
middleString = .join(tempWord)
scrambledWord = fLetter + middleString + lLetter
return scrambledWord
def getLetter(string, number, direction):
I found it very hard to understand what this function does. A better
name and a comment would help a lot. You might consider having separate
getFirstLetter() and getLastLetter() since much of getLetter() is taken
up with the conditionals and compensating for trying to do two things.
def getFirstLetter(string, number=0):
if string[number].isalpha() == True:
return (number, string[:number + 1])
else:
return getFirstLetter(string, number + 1)
Even better would be to use a simple loop to find the index of the first
letter, and split the string into three sections in the caller.
BTW thinking of writing this as a loop brings to mind some problems -
what will your program do with 'words' such as 555-1212 or Ha! ?
if direction == 'forward':
increment = 1
else:
increment = -1
if string[number].isalpha() == True:
if direction == 'forward':
return (number, string[:number + 1])
else:
return (number, string[number:])
elif string[number].isalpha() == False:
return getLetter(string, number + increment, direction)
if __name__ == __main__:
try:
if sys.argv[1].isspace() == True:
print No file was given to the program to
process.\n-- Program quitting --\n
else:
try:
f = open(sys.argv[1])
fileScramble(f)
f.close()
except IOError:
print That file does not exist, or you do not have
permission to access it.\n-- Program quitting --\n
except IndexError:
print No file was given to the program to
process.\n-- Program quitting --\n
___
Tutor maillist - Tutor@python.org
http://mail.python.org/mailman/listinfo/tutor
___
Tutor maillist - Tutor@python.org
http://mail.python.org/mailman/listinfo/tutor
[Tutor] Code review
If anyone has the time to look through an entire script, I would would be very grateful for any comments, tips or suggestions on a wiki-engine script I am working on. http://www.waywood.co.uk/cgi-bin/monkeywiki.py (this will download rather than execute) It does work, but I have not been using Python very long, and am entirely self-taught in computer programming of any sort, so I have huge doubts about my 'style'. I am also aware that I probably don't 'think like a programmer' (being, in fact a furniture maker!) I did post a previous version of this about a year(?) ago, and received some very welcome suggestions, but I have changed it quite a lot since then. Also, please ignore the licensing stuff - I do intend to make the thing available like this when I am more confident about it, and I am just getting a bit ahead of myself: you guys are the first people who know it's there. Many thanks ___ Tutor maillist - Tutor@python.org http://mail.python.org/mailman/listinfo/tutor
