improve this newbie code/nested functions in Python?

[Esmail]

Hi,

I'm new to writing Python code. This is a simple client I wrote, it
works, but I feel it doesn't look as clean as it could. Can anyone
make suggestions how to streamline this code?

Also, I am using two nested functions, it seems that nested functions
aren't used that much in Python - is that correct? And if so, how
come?

thanks,

Esmail

ps: I realize there is minimal error checking/exception handling.

#!/usr/bin/env python


import sys
from socket import *
from threading import Thread


class Client(object):
def __init__(self, host="localhost", port=5555, name = "esmail"):
self._host = host
self._port = port
self._name = name
self._address=(self._host, self._port)
self._sock=socket(AF_INET, SOCK_STREAM)
self._sock.connect(self._address)
self._parent = self
self._keepGoing = True

def info(self):
return self._host, self._port, self._name



class Listener(Thread):
def __init__(self, parent, tname="listener"):
Thread.__init__(self,name = tname)
self._parent = parent
print self._parent._host

def run(self):

while self._parent._keepGoing:
m = self._parent._sock.recvfrom(1024)
print m[0]



class Speaker(Thread):
def __init__(self, parent, tname = "speaker"):
Thread.__init__(self,name = tname)
self._parent = parent
self._parent._sock.send(self._parent._name + "\n")


def run(self):

while(True):
m = raw_input("-> ")
if m == "bye":
self._parent._sock.send(self._parent._name + " is
signing off.\n")
self._parent._sock.send("bye\n")
self._parent._keepGoing = False
break;
else:
self._parent._sock.send(m + "\n")


def main():

if len(sys.argv) == 4: # prog name + 3 args
host = sys.argv[1]
port = int(sys.argv[2])
name = sys.argv[3]
c = Client(host, port, name)
else:
c = Client()

print "Client connecting to - host=%s port=%d name=%s" % c.info
()

s = Client.Speaker(c)
s.start()

l = Client.Listener(c)
l.start()


main()

 

[Terry Reedy]

Hi,

I'm new to writing Python code. This is a simple client I wrote, it
works, but I feel it doesn't look as clean as it could. Can anyone
make suggestions how to streamline this code?

Also, I am using two nested functions, it seems that nested functions
aren't used that much in Python - is that correct? And if so, how
come?

What you wrote are two nested classes, not functions. In my opinion,
neither should be nested. Nothing is gained and something is lost.
Neither are used by client; indeed both use client.

Nested classes are rare because they have little use and are almost
never necessary.

Nested functions are a different story.

I would rename '_parent' (misleading) as 'client' or 'user'.

Terry Jan Reedy

ps: I realize there is minimal error checking/exception handling.

#!/usr/bin/env python


import sys
from socket import *
from threading import Thread


class Client(object):
def __init__(self, host="localhost", port=5555, name = "esmail"):
self._host = host
self._port = port
self._name = name
self._address=(self._host, self._port)
self._sock=socket(AF_INET, SOCK_STREAM)
self._sock.connect(self._address)
self._parent = self

Makes no sense in the code given.

self._keepGoing = True

def info(self):
return self._host, self._port, self._name



class Listener(Thread):
def __init__(self, parent, tname="listener"):
Thread.__init__(self,name = tname)
self._parent = parent
print self._parent._host

def run(self):

while self._parent._keepGoing:
m = self._parent._sock.recvfrom(1024)
print m[0]



class Speaker(Thread):
def __init__(self, parent, tname = "speaker"):
Thread.__init__(self,name = tname)
self._parent = parent
self._parent._sock.send(self._parent._name + "\n")


def run(self):

while(True):
m = raw_input("-> ")
if m == "bye":
self._parent._sock.send(self._parent._name + " is
signing off.\n")
self._parent._sock.send("bye\n")
self._parent._keepGoing = False
break;
else:
self._parent._sock.send(m + "\n")


def main():

if len(sys.argv) == 4: # prog name + 3 args
host = sys.argv[1]
port = int(sys.argv[2])
name = sys.argv[3]
c = Client(host, port, name)
else:
c = Client()

print "Client connecting to - host=%s port=%d name=%s" % c.info
()

s = Client.Speaker(c)
s.start()

l = Client.Listener(c)
l.start()


main()

 

[Esmail]

Hi!

What you wrote are two nested classes, not functions.  

Ooops .. yes of course .. simple mistake (it was late ..

In my opinion,
neither should be nested.  Nothing is gained and something is lost.
Neither are used by client; indeed both use client.

I nested them because I see them as components of the client which
keeps track of the connection parameters and makes the initial
connection and then hands the info off to the two threads for
processing, and then also helps the two threads communicate with
each other.

This seemed like a good choice to me, can you (or someone else)
elaborate why this is not a good design? The idea was to encapsulate
all the client info/code in one place.

I would rename '_parent' (misleading) as 'client' or 'user'.

good suggestion .. I chose parent or could have chosen "outer" since
I was looking for a way for the nested classes to access the
outer class

The series of assignments in __init__ in client was to store the
connection parameters as instance variables in the outer client
class. the __keepGoing__ variable is used to help the two threads
communicate with each other and know when to shut down.

This feedback is exactly the sort of thing I was looking for, thanks,
I'm looking forward to more suggestions.

Esmail

 

[pruebauno]

Hi,

I'm new to writing Python code. This is a simple client I wrote, it
works, but I feel it doesn't look as clean as it could. Can anyone
make suggestions how to streamline this code?

Also, I am using two nested functions, it seems that nested functions
aren't used that much in Python - is that correct? And if so, how
come?

thanks,

Esmail

ps: I realize there is minimal error checking/exception handling.

#!/usr/bin/env python

import sys
from socket import *
from threading import Thread

class Client(object):
    def __init__(self, host="localhost", port=5555, name = "esmail"):
        self._host = host
        self._port = port
        self._name = name
        self._address=(self._host, self._port)
        self._sock=socket(AF_INET, SOCK_STREAM)
        self._sock.connect(self._address)
        self._parent = self
        self._keepGoing = True

    def info(self):
        return self._host, self._port, self._name

    class Listener(Thread):
       def __init__(self, parent, tname="listener"):
           Thread.__init__(self,name = tname)
           self._parent = parent
           print self._parent._host

       def run(self):

           while self._parent._keepGoing:
               m = self._parent._sock.recvfrom(1024)
               print m[0]

    class Speaker(Thread):
       def __init__(self, parent, tname = "speaker"):
           Thread.__init__(self,name = tname)
           self._parent = parent
           self._parent._sock.send(self._parent._name + "\n")

       def run(self):

           while(True):
               m = raw_input("-> ")
               if m == "bye":
                   self._parent._sock.send(self._parent._name + " is
signing off.\n")
                   self._parent._sock.send("bye\n")
                   self._parent._keepGoing = False
                   break;
               else:
                   self._parent._sock.send(m + "\n")

def main():

    if len(sys.argv) == 4:  # prog name + 3 args
        host = sys.argv[1]
        port = int(sys.argv[2])
        name = sys.argv[3]
        c = Client(host, port, name)
    else:
        c = Client()

    print "Client connecting to - host=%s  port=%d   name=%s" % c.info
()

    s = Client.Speaker(c)
    s.start()

    l = Client.Listener(c)
    l.start()

main()

How about renaming the Client to SocketConfig, Listener to
ClientListener and Speaker to ClientSpeaker and put all at the same
level. The you can do this:

c = SocketConfig(host, port, name)
s = ClientSpeaker(c)
l = ClientListener(c)

the other option would be to create a Speaker and Listener factory in
Client that returns Speakers and Listeners so you can do:

c = Client(host, port, name)
s = c.Speaker()
l = c.Listener()

 

[Esmail]

Hi,

I'm new to writing Python code. This is a simple client I wrote, it
works, but I feel it doesn't look as clean as it could. Can anyone
make suggestions how to streamline this code?

Also, I am using two nested functions, it seems that nested functions
aren't used that much in Python - is that correct? And if so, how
come?


ps: I realize there is minimal error checking/exception handling.

#!/usr/bin/env python

import sys
from socket import *
from threading import Thread

class Client(object):
    def __init__(self, host="localhost", port=5555, name = "esmail"):
        self._host = host
        self._port = port
        self._name = name
        self._address=(self._host, self._port)
        self._sock=socket(AF_INET, SOCK_STREAM)
        self._sock.connect(self._address)
        self._parent = self
        self._keepGoing = True

    def info(self):
        return self._host, self._port, self._name

    class Listener(Thread):
       def __init__(self, parent, tname="listener"):
           Thread.__init__(self,name = tname)
           self._parent = parent
           print self._parent._host

       def run(self):

           while self._parent._keepGoing:
               m = self._parent._sock.recvfrom(1024)
               print m[0]

    class Speaker(Thread):
       def __init__(self, parent, tname = "speaker"):
           Thread.__init__(self,name = tname)
           self._parent = parent
           self._parent._sock.send(self._parent._name + "\n")

       def run(self):

           while(True):
               m = raw_input("-> ")
               if m == "bye":
                   self._parent._sock.send(self._parent._name + " is
signing off.\n")
                   self._parent._sock.send("bye\n")
                   self._parent._keepGoing = False
                   break;
               else:
                   self._parent._sock.send(m + "\n")

def main():

    if len(sys.argv) == 4:  # prog name + 3 args
        host = sys.argv[1]
        port = int(sys.argv[2])
        name = sys.argv[3]
        c = Client(host, port, name)
    else:
        c = Client()

    print "Client connecting to - host=%s  port=%d   name=%s" % c.info
()

    s = Client.Speaker(c)
    s.start()

    l = Client.Listener(c)
    l.start()

main()

How about renaming the Client to SocketConfig, Listener to
ClientListener and Speaker to ClientSpeaker and put all at the same
level. The you can do this:

 c = SocketConfig(host, port, name)
 s = ClientSpeaker(c)
 l = ClientListener(c)

the other option would be to create a Speaker and Listener factory in
Client that returns Speakers and Listeners so you can do:

c = Client(host, port, name)
s = c.Speaker()
l = c.Listener()

Ah .. I like both .. will probably do the first one (easier) but
keep the 2nd one in mind. Thanks!!

Esmail

 

[Terry Reedy]

I nested them because I see them as components of the client which
keeps track of the connection parameters and makes the initial
connection and then hands the info off to the two threads for
processing, and then also helps the two threads communicate with
each other.

This seemed like a good choice to me, can you (or someone else)
elaborate why this is not a good design? The idea was to encapsulate
all the client info/code in one place.

They are all encapsulated in the module.

To make a closure, the inner function *must* be nested in the outer.
To be an instance method, a function *must* be a class attribute, and
the easier way to indicate that is by nesting.

In this case, the client does *not* use the other two classes, so the
nesting is misleading. I think the only time a class *might* be nested
is when it is used by and only (directly) used by whatever it is nested
in -- and even then, nesting is not necessary. A class statement can
only use global and class local names and not the non-global names in
the surrounding context.

tjr

 

[Esmail]

To make a closure, the inner function *must* be nested in the outer.
To be an instance method, a function *must* be a class attribute, and
the easier way to indicate that is by nesting.

In this case, the client does *not* use the other two classes, so the
nesting is misleading.  I think the only time a class *might* be nested
is when it is used by and only (directly) used by whatever it is nested
in -- and even then, nesting is not necessary.  A class statement can
only use global and class local names and not the non-global names in
the surrounding context.

Thanks Terry, you've gotten me something to ponder.

Regards,
Esmail

 

[Paul Hankin]

Hi,

I'm new to writing Python code. This is a simple client I wrote, it
works, but I feel it doesn't look as clean as it could. Can anyone
make suggestions how to streamline this code?

Also, I am using two nested functions, it seems that nested functions
aren't used that much in Python - is that correct? And if so, how
come?

thanks,

Esmail

ps: I realize there is minimal error checking/exception handling.

#!/usr/bin/env python

import sys
from socket import *
from threading import Thread

class Client(object):
    def __init__(self, host="localhost", port=5555, name = "esmail"):
        self._host = host
        self._port = port
        self._name = name
        self._address=(self._host, self._port)
        self._sock=socket(AF_INET, SOCK_STREAM)
        self._sock.connect(self._address)
        self._parent = self
        self._keepGoing = True

    def info(self):
        return self._host, self._port, self._name

    class Listener(Thread):
       def __init__(self, parent, tname="listener"):
           Thread.__init__(self,name = tname)
           self._parent = parent
           print self._parent._host

       def run(self):

           while self._parent._keepGoing:
               m = self._parent._sock.recvfrom(1024)
               print m[0]

    class Speaker(Thread):
       def __init__(self, parent, tname = "speaker"):
           Thread.__init__(self,name = tname)
           self._parent = parent
           self._parent._sock.send(self._parent._name + "\n")

       def run(self):

           while(True):
               m = raw_input("-> ")
               if m == "bye":
                   self._parent._sock.send(self._parent._name + " is
signing off.\n")
                   self._parent._sock.send("bye\n")
                   self._parent._keepGoing = False
                   break;
               else:
                   self._parent._sock.send(m + "\n")

def main():

    if len(sys.argv) == 4:  # prog name + 3 args
        host = sys.argv[1]
        port = int(sys.argv[2])
        name = sys.argv[3]
        c = Client(host, port, name)
    else:
        c = Client()

    print "Client connecting to - host=%s  port=%d   name=%s" % c.info
()

    s = Client.Speaker(c)
    s.start()

    l = Client.Listener(c)
    l.start()

main()

I would decouple the speaker and the listener from the client, and
make the client interface more abstract. Simple and descriptive
interfaces can make code dramatically easier to understand.

class ClientListener(Thread):
def __init__(self, client, ...):
...

def run(self):
while True:
m = self.client.receive_message()
print m

class ClientSpeaker(Thread):
def __init__(self, client):
client.connect()
...
def run(self):
while True:
m = raw_input()
if m == 'bye':
self.client.disconnect()
...
else:
self.client.send_message(m)

The 'connect' method can print the client name, the 'disconnect' can
say 'bye' and set _KeepGoing. receive_message can extract the
interesting bit from the socket read. Hope you get the idea.

This way, you don't use the inner workings of client in the Listener
and Speaker classes, and don't violate the 'law of demeter'. If
nothing else, it makes the speaker and listener much easier to read.
If you later want to test these classes, you'll find it a ton easier,
since you can mock the client class.. but perhaps that's a little too
advanced.

HTH

 

[Esmail]

I would decouple the speaker and the listener from the client, and
make the client interface more abstract. Simple and descriptive
interfaces can make code dramatically easier to understand.

class ClientListener(Thread):
def __init__(self, client, ...):
...

def run(self):
while True:
m = self.client.receive_message()
print m

class ClientSpeaker(Thread):
def __init__(self, client):
client.connect()
...
def run(self):
while True:
m = raw_input()
if m == 'bye':
self.client.disconnect()
...
else:
self.client.send_message(m)

The 'connect' method can print the client name, the 'disconnect' can
say 'bye' and set _KeepGoing. receive_message can extract the
interesting bit from the socket read. Hope you get the idea.

This way, you don't use the inner workings of client in the Listener
and Speaker classes, and don't violate the 'law of demeter'. If
nothing else, it makes the speaker and listener much easier to read.
If you later want to test these classes, you'll find it a ton easier,
since you can mock the client class.. but perhaps that's a little too
advanced.

Hi Paul,

Yes that was very helpful -- I'm glad I found this group

Best,
Esmail