[Tutor] Re: Tutor digest, Vol 1 #2328 - 13 msgs

[Charlie Clark]

On 2003-03-22 at 00:39:04 [+0100], tutor-request@python.org wrote:
> I have a program which I have begun writing,
> and the code works but is not in appropriate OOP format (or doesn't seem 
> to be). I am new to writing Python and OOP, having written C programs and 
> the like for several years. If any of you experienced Python programmers 
> could take a quick look and advise me on how to get this program into the 
> appropriate style/format
> before it gets so large that it is a major job to
> revamp it, I would greatly appreciate it.

Hi Vicki,

I don't count among "great programmers of our time" and OOP isn't something 
I'm very good at but I hope the following comments are useful.

"input" is a reserved word in Python so it's best not to use it unless you 
have a good reason. Python won't complain but it might confuse people who 
read your code later. "src" is shorter anyway ;-)

I think this whole thing should be a class or a module. In Python it won't 
make any difference when you call it but I imagine you might want to pack 
more of such classes in a single module.

You should learn to use """ long
strings""" instead of concatenation. I think you should also peruse the 
suggested syntax rules on Python.org; I find your source somewhat 
inconsistent re. capitalising or use of "_" to name functions and 
variables. I also note that you want to "print" lots of variables before 
you check for them. Why do you "print" rather than "return"? This may well 
lead to several errors at once making it harder to find them.

def SendCommand(command):
    for command in ('\x09','\x06'):
	port = serial.Serial(0, 9600, 8, 'N', 2, timeout=2)
	port.write(command)
	old=outputbox.getvalue()  # where does outputbox come from?
	new=string.join([old, hex(ord(command))])
	outputbox.setvalue(new)  # this might cause an error due to scope
	input=port.read()
	print input
	if input:
		returnedval=hex(ord(input))
		if returnedval:
			print returnedval
			old=outputbox.getvalue()
			new=string.join([old, returnedval])
			outputbox.setvalue(new)
	port.close()

I don't know what outputbox is but you might get an error doing something 
to it within a function when you haven't explicitly passed it into that 
function.

I'd be tempted to rewrite this:

src = self.port.read()
if src:
	print src
	val = hex(ord(src))
	if val:
		print val
		old = outputbox.getvalue()
		new = "".join([old, val])
		outputbox.setvalue(new)
self.port.close()

Checking twice first for "src" and then for "val" seems wrong to me. I'd 
favour
src = self.port.read()
try:
	val = hex(ord(src)) 
	old = outputbox.getvalue()
	new = "".join([old, val])
	outputbox.setvalue(new) # shouldn't this be returned?
	return src, val
except Error, msg
	return msg, "something went wrong"
self.port.close()

Note that I've adjusted your syntax to make it more Pythony. Reviewing the 
first part of this function/method suggests further improvements are 
possible, ie. assigning "old", creating "new", assigning outputbox.

I see that you define "port" the same in two separate functions. This is a 
nopey no as it means "copy + paste" to change it. I think this another 
reason for using a class. This is untested and subject to correction by 
gurus.

class Callback:
	def port(self):
		return serial.Serial(0, 9600, 8, 'N', 2, timeout=2)

	def SendCommand(self, command):
    	for command in ('\x09','\x06'):
			myport = self.port()

	def ReadAndClearStatus(self):
    	myport = self.port()

I hope this helps.

Charlie