improve this newbie code/nested functions in Python?

Discussion in 'Python' started by Esmail, Mar 20, 2009.

  1. Esmail

    Esmail Guest

    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()
    Esmail, Mar 20, 2009
    #1
    1. Advertising

  2. Esmail

    Terry Reedy Guest

    Esmail wrote:
    > 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()
    > --
    > http://mail.python.org/mailman/listinfo/python-list
    >
    Terry Reedy, Mar 20, 2009
    #2
    1. Advertising

  3. Esmail

    Esmail Guest

    Hi!

    On Mar 20, 1:06 am, Terry Reedy <> wrote:
    >
    > 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
    Esmail, Mar 20, 2009
    #3
  4. Esmail

    Guest

    On Mar 19, 10:21 pm, Esmail <> wrote:
    > 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()
    , Mar 20, 2009
    #4
  5. Esmail

    Esmail Guest

    On Mar 20, 2:02 pm, wrote:
    > On Mar 19, 10:21 pm, Esmail <> wrote:
    >
    >
    >
    > > 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()



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

    Esmail
    Esmail, Mar 20, 2009
    #5
  6. Esmail

    Terry Reedy Guest

    Esmail wrote:

    >> 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.


    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
    Terry Reedy, Mar 20, 2009
    #6
  7. Esmail

    Esmail Guest


    > 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
    Esmail, Mar 20, 2009
    #7
  8. Esmail

    Paul Hankin Guest

    On Mar 20, 3:21 am, Esmail <> wrote:
    > 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
    --
    Paul Hankin
    Paul Hankin, Mar 21, 2009
    #8
  9. Esmail

    Esmail Guest

    Paul Hankin wrote:
    >
    >
    > 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

    > --
    > Paul Hankin
    > --
    > http://mail.python.org/mailman/listinfo/python-list
    >
    Esmail, Mar 21, 2009
    #9
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. S Kemplay

    How can I improve these functions?

    S Kemplay, Apr 4, 2004, in forum: C Programming
    Replies:
    2
    Views:
    307
    S Kemplay
    Apr 5, 2004
  2. Gustavo Campanelli

    Newbie question: Any way to improve this code?

    Gustavo Campanelli, Dec 5, 2003, in forum: Python
    Replies:
    10
    Views:
    438
    Gustavo Campanelli
    Dec 7, 2003
  3. Michael Schwarz
    Replies:
    1
    Views:
    182
    Rouslan Korneychuk
    Nov 6, 2012
  4. Terry Reedy
    Replies:
    0
    Views:
    130
    Terry Reedy
    Nov 2, 2012
  5. Stefan H. Holek
    Replies:
    0
    Views:
    126
    Stefan H. Holek
    Nov 2, 2012
Loading...

Share This Page