Nastiness with global vars. How do I avoid?
Robin Munn
rmunn at pobox.com
Wed Nov 27 19:12:11 EST 2002
Rich Daley <rich at DELETE_THISowl.me.uk> wrote:
> Hi all.
Hi yourself. :-)
>
> I attach the beginnings of a piece of code which will become an ewtoo
><http://www.ewtoo.org> style chat room.
>
> However, at the moment I use a global dictionary to hold the users' names,
> and I realise that as the code gets bigger and longer (and more Object
> Oriented) it's going to be harder and harder to maintain it this way.
Good thinking. Globals make for difficult code maintenance.
> Does anyone have any idea how I could use a Class instead to acheive the
> same functionality? Ideally I'd love to have classes for all the different
> kinds of methods and stuff too, but this is the most critical bit.
>
> I'm not sure I made sense, so please ask me if this doesn't make sense...
>
> Thanks in advance for your help!
>
> ~ Rich
>
>
> ------BEGIN CODE main.py------
Right. Let's get to work. *rolls up sleeves*
> #!/usr/bin/python
> import SocketServer,socket,string,time
>
> users = {}
>
> class TalkRequestHandler(SocketServer.BaseRequestHandler):
> def handle(self):
> global users
At this point I'm wondering why you don't just make this an instance
variable, like so:
class TalkRequestHandler(SocketServer.BaseRequestHandler):
def __init__(self):
SocketServer.BaseRequestHandler.__init__(self) # Call parent first
self.users = {}
def some_func_that_uses_the_users_var(self):
if self.users.has_key('foo'):
print 'Ah ha!'
OK, back to your code.
> self.user_connect()
> stillhere = 1
> while 1:
> if not stillhere: break
> msg = self.request.recv(1024)
> if not msg: break
> stillhere = self.process(msg)
You could save a line of code by moving one of those tests:
while 1:
msg = self.request.recv(1024)
if not msg: break
stillhere = self.process(msg)
if not stillhere: break
This would allow you to avoid setting stillhere outside your while loop,
which feels cleaner to me.
> self.user_disconnect()
>
> def user_connect(self):
> global users
> self.send_user_unterminated('Enter your name: ')
> name = self.request.recv(1024)
> self.name = name.strip()
> if not self.name.isalpha():
> self.send_user('Sorry, name must be only letters')
> return
> if users.has_key(self.name):
> self.send_user('Sorry, name in use')
> return
> self.send_all('+++ '+self.name+' connected!')
> users[self.name] = self.request
> self.look()
So far I don't see any problems with using an instance variable instead
of a global.
>
> def user_disconnect(self):
> global users
> del(users[self.name])
> self.send_all('--- '+self.name+' disconnected!')
>
> def send_user(self,msg):
> self.request.send(msg + '\n')
>
> def send_user_unterminated(self,msg):
> self.request.send(msg)
>
> def send_all(self,msg):
> for user in users:
> users[user].send(msg + '\n')
>
> def process(self,msg):
> command = ""
> args = ""
> if msg[0].isalpha():
> command_completed = 0
> for char in msg:
> if (not command_completed):
> if char.isalpha(): command += char
> else:
> command_completed = 1
> args += char
> else: args += char
> else:
> command_completed = 0
> for char in msg:
> if (not command_completed):
> if char.isalpha() or char.isspace():
> command_completed = 1
> args += char
> else: command += char
> else: args += char
>
> command = command.strip().lower()
> args = args.strip()
This looks unnecesasrily complicated and error-prone. I couldn't grok
its purpose on my first reading or even my second reading. Therefore,
six months from now when you come back to maintain this code, you won't
be able to understand it either. You need some comments here. Always
remember: write your code as if it's going to be read by a stranger --
because six months from now, that stranger will be you!
If I understand what you're doing correctly, you're trying to grab the
first token you find into command and the rest of the string into args.
This might be a place where you want to use regexps. As fond as I am of
quoting that Jamie Zawinski quote, sometimes regexps have their uses.
The quote I'm referring to, by the way, is:
Some people, when confronted with a problem, think "I know, I'll use
regular expressions." Now they have two problems.
Here, I think you want something like this:
if msg[0].isalpha():
pattern = re.compile(r'^([a-zA-Z]+)([^a-zA-Z].*)$')
result = pattern.match(msg)
command = result.group(1)
args = result.group(2)
else:
pattern = re.compile(r'^([^a-zA-Z ]+)([a-zA-Z ].*)$')
result = pattern.match(msg)
command = result.group(1)
args = result.group(2)
However, I would recommend making a small change: instead of accepting
only alphabetic characters as commands, accept alphanumerics (letters,
numbers and underscores). This will make your regular expressions even
easier to read, since there's a shorthand:
if msg[0].isalphanum():
pattern = re.compile(r'^(\w+)(\W.*)$')
result = pattern.match(msg)
command = result.group(1)
args = result.group(2)
else:
pattern = re.compile(r'^([^\w\s]+)([\w\s].*)$')
result = pattern.match(msg)
command = result.group(1)
args = result.group(2)
This is even easier to read and understand: \w means an alphanumeric
character (see http://www.python.org/doc/current/lib/re-syntax.html) and
\W means anything non-alphanumeric. \s means whitespace, \S means
anything non-whitespace.
But this is still wrong, since it requires at least one character to
follow the command, which might not be the case if the command is, for
example, "who". So:
if msg[0].isalphanum():
pattern = re.compile(r'^(\w+)(.*)$')
result = pattern.match(msg)
command = result.group(1)
args = result.group(2)
else:
pattern = re.compile(r'^([^\w\s]+)(.*)$')
result = pattern.match(msg)
command = result.group(1)
args = result.group(2)
This takes advantage of the fact that regular expression wildcards (like
+ and *) are "greedy" -- they will match as much as possible. So if the
whole of msg matches the first part of the pattern, then the second part
will be empty. But that's OK, since the * match means 0 or more. Thus,
args will be an empty string, which is what you would want.
My own rule of thumb for whether a regular expression is the right tool
for the job: if you can understand the regexp within about 15 seconds,
use it. Otherwise, look for a way to simplify it. This regexp is
understandable within 5 seconds, so it's O.K.
>
> #self.send_user("Command: "+ command+" Args: "+args)
>
> if command == "'" or command == '"' or command == 'say':
> self.say(args)
> elif command == ';' or command == ':' or command == 'emote':
> self.send_all(self.name+" "+args)
> elif command == '~' or command == 'think':
> self.send_all(self.name+" thinks . o O ( "+args+ " )")
> elif command == ')' or command == 'sing':
> self.send_all(self.name+" sings o/~ "+args+" o/~")
> elif command == 'hug':
> self.hug(args)
> elif command == 'who':
> self.who()
> elif command == 'look':
> self.look()
> elif command == 'quit':
> self.send_user('Thanks for visiting! Come back soon!')
> return 0
> else: self.send_user("I don't know how to "+command+
> ". I'm sorry but I'm very primitive.")
> return 1
Ah, the good old if..elif..else block. A sign of someone who wants to
write a switch() statement and doesn't know about the more elegant
alternatives, like (for example) dispatcher functions:
def do_say(self, args):
self.say(args)
def do_emote(self, args):
self.send_all(self.name+" "+args)
def do_think(self, args):
self.send_all(self.name+" thinks . o O ( "+args+ " )")
def do_sing(self, args):
self.send_all(self.name+" sings o/~ "+args" o/~")
def do_hug(self, args):
self.hug(args)
def do_who(self, args):
self.who(args)
def do_look(self, args):
self.look(args)
def do_quit(self, args):
self.send_user('Thanks for visiting! Come back soon!')
raise QuitException
def do_ERROR(self, args):
self.send_user("I don't know how to "+command+
". I'm sorry but I'm very primitive.")
raise UnknownCommand
def dispatch(self, command, args):
command = command.lower()
special_cases = {
"'": do_say,
'"': do_say,
';': do_emote,
':': do_emote,
'~': do_think,
')': do_sing
}
try:
if special_cases.has_key(command):
special_cases[command](args)
elif hasattr(self, 'do_' + command):
getattr(self, 'do_' + command)(args)
else:
self.do_ERROR(args)
except QuitException:
return 0
except UnknownCommand:
return 1
This approach has the added advantage that when you want to add a new
command, you don't need to touch the dispatch() function. All you have
to do is add a new do_whatever() method and voila!
>
> def look(self):
Actually, instead of my stub-like do_look() above, just change this
function to be called do_look(), make sure it accepts two arguments
(self and args) even though you won't be using args, and now you have a
function that's really to be called from the dispatcher.
> global users
Still no reason not to use an instance variable here.
> lookstr = "Welcome to Talk! This talker is in very early stages\n"
> lookstr += "of development, and as a result it's pretty crap.\n"
> lookstr += "Please, take a look around though :)\n\n"
> if len(users) == 1:
> lookstr += "You're on your own here :(\n"
> else:
> i = 0
> userstr = ""
> for user in users:
> if user != self.name:
> i += 1
> userstr += user + "\n"
> lookstr += "You are here with the following " + `i` + " people:\n"
> lookstr += userstr
> self.send_user(lookstr)
>
> def who(self):
> global users
> whostring = "---WHO---\n"
> for user in users:
> whostring += user + "\n"
> whostring += "---------"
> self.send_user(whostring)
>
> def say(self,line):
> if line.endswith('?'):
> self.send_all(self.name+" asks '"+ line + "'")
> elif line.endswith('!'):
> self.send_all(self.name+" exclaims '"+ line + "'")
> else:
> self.send_all(self.name+" says '"+ line + "'")
>
> def hug(self,user):
> global users
> if user.lower() in map(string.lower,users.keys()):
> self.send_all("* "+self.name+" extends arms and hugs "+user)
> else: self.send_user("No user called "+user+" logged in")
>
> class TalkServer (SocketServer.ThreadingTCPServer):
> pass
>
> addr = ('0.0.0.0', 4321)
>
> TalkServer.allow_reuse_address = 1
>
> server = TalkServer(addr, TalkRequestHandler)
>
> server.serve_forever()
> ------END CODE main.py------
I'm running out of time here, so I'll stop my comments. I hope this has
helped you get some ideas for writing cleaner, more maintainable code.
--
Robin Munn <rmunn at pobox.com>
http://www.rmunn.com/
PGP key ID: 0x6AFB6838 50FF 2478 CFFB 081A 8338 54F7 845D ACFD 6AFB 6838
More information about the Python-list
mailing list