Critic my module

[Steven D'Aprano]

As requested, some constructive criticism of your module.

On Thu, 25 Jul 2013 09:24:30 -0400, Devyn Collier Johnson wrote:

> #!/usr/bin/python3
> #Made by Devyn Collier Johnson, NCLA, Linux+, LPIC-1, DCTS

What's NCLA, Linux+, LPIC-1, DCTS? Do these mean anything? Are we 
supposed to know what they mean?

"Made by" has no legal significance. You probably want:

Copyright © 2013 Devyn Collier Johnson.


> #Made using the Geany IDE

Nobody gives a monkey's toss what editor you used to type up the module. 
You might as well mention the brand of monitor you used, or whether the 
keyboard is Dvorak or Qwerty. 


> #LGPLv3

You can't just drop in a mention of "LGPLv3" and expect it to mean 
anything. You actually have to obey the licence yourself, and that 
includes *actually including the licence in your work*. (You're 
technically in violation of the licence at the moment, however since the 
only person whose copyright you are infringing is yourself, it doesn't 
matter. However anyone else using your code is at risk.)

http://www.gnu.org/licenses/gpl-howto.html

In the case of the LGPL, you have to include the text of *both* the GPL 
and the LGPL, not just one.



> import re, sys, subprocess, platform
> def grep(regex,textf):
> #Sample Command: grep.grep("^x",dir()) #Syntax:
> boash.grep(regexp_string,list_of_strings_to_search)

Comments using # are only of use to people reading the source code. If 
you want comments to be available at the interactive prompt, you should 
write them as doc strings:

def grep(regex, textf):
    """This string is a docstring.

    Sample command: ...
    Blah blah blah
    """

Then, at the interactive prompt, the user can say:

help(boash.grep)

to read the docstring.


> 	version = '0.2a'

That's quite useless, since it is a local variable invisible outside of 
the function.

Also, why would you bother giving every individual function a version 
number? That's rather pointless. The user cannot pick and choose function 
A with version number 0.6 and function B with version number 0.7 if the 
module provides versions 0.7 of both.


> 	expr = re.compile(regex)
> 	match = re.findall(expr, textf)
> 	if match != None:
> 		print(match)

When comparing with None, it is preferred to use "is" and "is not" rather 
than equality tests.


> def ls():
> 	version = '0.3'
> 	print(subprocess.getoutput('ls'))
> def dir():
> 	version = '0.3'
> 	print(subprocess.getoutput('dir'))

A blank line or two between functions does wonders for readability. There 
is no prize for conserving newlines.

You might like to read PEP 8, the Python style guide. It is optional, but 
still makes a very good guide.

http://www.python.org/dev/peps/pep-0008/


> def bash(*arg):
> 	version = '0.3'
> 	print(subprocess.getoutput(arg))
> def shell(*arg):
> 	version = '0.3'
> 	print(subprocess.getoutput(arg))

bash is not a synonym for "shell". "The shell" might be sh, csh, bash, or 
any one of many other shells, all of which are slightly (or not so 
slightly) different.


> def clear_bash_history():
> 	version = '0.3'
> 	print(subprocess.getoutput('history -c'))
[...]

Do you really need ten aliases for 'history -c'?

If you want to define aliases for a function, don't recreate the entire 
function ten times. Start with defining the function once, then:

clear_bash_hist = clear_hist = clear_history = clear_bash_history

etc. But really, having ten names for the one function just confuses 
people, who then wonder what subtle difference there is between 
delete_history and clear_history.

> def firefox():
> 	version = '0.3'
> 	print(subprocess.Popen('(firefox &)'))

Is Firefox really so important that it needs a dedicated command?

What about Debian users? Doesn't Iceweasel get a command?


> def xterm():
> 	version = '0.3'
> 	print(subprocess.Popen('(xterm &)'))

Surely the user already has an xterm open, if they are running this 
interactively? Why not just use your xterm's "new window" or "new tab" 
command?


[...delete more trivial calls to subprocess...]

> def repeat_cmd():
> 	version = '0.3'
> 	print(subprocess.Popen('!!'))
[... delete two exact copies of this function...]

> def ejcd():
> 	version = '0.3'
> 	print(subprocess.Popen('eject cdrom1'))
[... delete FOURTEEN exact copies of this function...]

Really? Is anyone going to type "eject_disc_tray" instead of "eject"?


I think that will do.

This doesn't really do anything except define a large number of trivial 
wrappers to commands already available in the shell. Emphasis on the 
*trivial* -- with the exception of the grep wrapper, which is all of four 
lines (ignoring the useless internal version number), every single one of 
these wrapper functions is a one-liner.[1] In other words, you're not 
adding any value to the shell commands by wrapping them in Python. There 
are plenty of big, complex shell commands that take a plethora of options 
and could do with some useful Python wrappers, like wget. But you haven't 
done them.

Nor have you added extra security, or even extra convenience. You've done 
nothing that couldn't be done using the shell "alias" command, except in 
Python where the syntax is less convenient (e.g. "ls" in the shell, 
versus "ls()" in Python).




[1] I think every newbie programmer goes through a stage of pointlessly 
writing one-liner wrappers to every second function they see. I know I 
did. The difference is, before the Internet, nobody did it publicly.


-- 
Steven