3
\$\begingroup\$

I've written the following code, but I need some review on design style. I want to develop a library/module and expose an API to client programs. How do I do that?

The code just runs a TCP server listening for incoming messages and another thread accepting user input to send messages.

from socket import AF_INET, SOCK_DGRAM
import sys;
import socket;
import select;
import thread;
import pickle;
import struct;
import time;
import threading;


class AgTime:
    lClock=0;
    pClock=0;
    count=0;

#NTP client object that queries NTP server for time.
class Ntp:

    def __init__(self):
        # # Set the socket parameters 
        self.host = "pool.ntp.org"
        self.port = 123
        self.buf = 1024
        self.address = (self.host,self.port)
        self.msg = '\x1b'+47*'\0'
        # reference time (in seconds since 1900-01-01 00:00:00)
        self.TIME1970 = 2208988800 # 1970-01-01 00:00:00

    def time(self):
        # connect to server
        client = socket.socket( AF_INET, SOCK_DGRAM)
        client.sendto(self.msg, self.address)
        data, address = client.recvfrom( self.buf )

        retval=struct.unpack("!12I",data);
        t = retval[10]
        t -= self.TIME1970
        return t;

class server(threading.Thread):
    def __init__(self,portNo):
        threading.Thread.__init__(self);
        self.PORTNO=portNo;
        self.server_socket=socket.socket(socket.AF_INET,socket.SOCK_STREAM);
        self.server_socket.bind(("",self.PORTNO));
        self.CONNECTION_LIST=[];
        self.RECV_BUFFER=4096;

    def run(self):
        self.server_socket.listen(10);
        self.CONNECTION_LIST.append(self.server_socket);
        print "Server started on port :"+str(self.PORTNO);

        while(1):
            read_sockets,write_sockets,error_sockets=select.select(self.CONNECTION_LIST,[],[]);
            for sock in read_sockets:
                if sock==self.server_socket:
                    sockfd, addr= self.server_socket.accept();
                    self.CONNECTION_LIST.append(sockfd);
                    print "Message rxd from (%s %s)",addr
                else:
                    try:
                        data=pickle.loads(sock.recv(self.RECV_BUFFER));
                        if data:
                            thread.start_new_thread(recv,(data,));
                    except:
                        sock.close();
                        self.CONNECTION_LIST.remove(sock);
                        continue;
        self.server_socket.close();

def recv(data):
    lock.acquire();
    global time_stamp;
    t_message=data;
    t_stamp= {'pClock':0, 'lClock':0,'count':0};

    #recv part of algorithm starts from here
    t_stamp['lClock']=max(time_stamp['lClock'],t_message['lClock'],time_stamp['pClock']);
    if(t_stamp['lClock']==t_message['lClock'] and t_stamp['lClock']==time_stamp['lClock']):
        t_stamp['count']=max(time_stamp['count'],t_message['count'])+1;
    elif (t_stamp['lClock']==time_stamp['lClock']):
        t_stamp['count']=time_stamp['count']+1;
    elif(t_stamp['lClock']==t_message['lClock']):
        t_stamp['count']=t_message['count']+1;
    else:
        t_stamp['count']=0;

    time_stamp['lClock']=t_stamp['lClock'];
    time_stamp['pClock']=t_stamp['pClock'];
    time_stamp['count']=t_stamp['count'];
    print data;
    lock.release();


def send(data,portno):
    lock.acquire();
    global time_stamp;
    t_stamp={'pClock':0, 'lClock':0,'count':0};
    t_stamp['lClock']=max(time_stamp['lClock'],time_stamp['pClock']);
    if(t_stamp['lClock']==time_stamp['lClock']):
        time_stamp['count']=time_stamp['count']+1;
    else:
        time_stamp['count']=0;
    t_stamp['pClock']=time_stamp['pClock'];
    t_stamp['count']=time_stamp['count'];

    #update the time_stamp object to hold the event timestamp
    time_stamp['lClock']=t_stamp['lClock'];
    time_stamp['pClock']=t_stamp['pClock'];
    time_stamp['count']=t_stamp['count'];
    client_socket=socket.socket(socket.AF_INET, socket.SOCK_STREAM);
    client_socket.connect(("localhost",portno));
    client_socket.send(pickle.dumps(t_stamp));
    client_socket.close();
    lock.release();


if __name__ == "__main__":
    #create a global lock for mutual exclusion
    global lock;
    lock=threading.RLock();
        #create an ntp object
    ntp=Ntp();
    #create a time-stamp object
    global time_stamp;
    time_stamp= {'pClock':0, 'lClock':0,'count':0};
        #launch a server thread
    server_thread=server(int(sys.argv[1]));
    server_thread.start();
    while 1:
        data=raw_input("Enter some text to send");
        portno=raw_input("Enter portno");
        if(data!="quit"):
            thread.start_new_thread(send,(data,int(portno)));   
        else:
            break;
        print "Exiting";
\$\endgroup\$
  • 1
    \$\begingroup\$ I have the feeling that the AgTime and the Ntp classes are not actually useful. Also, the indentation of the last line looks wrong to me. \$\endgroup\$ – SylvainD Feb 13 '14 at 9:30
  • \$\begingroup\$ AgTime is not useful but NTP class is. \$\endgroup\$ – raka Feb 14 '14 at 1:57
  • \$\begingroup\$ I do see where we create an instance of ntp but I cannot see where the time() fonction is used. (If I missed something, let's pretend it's because I am not fully awaken yet). \$\endgroup\$ – SylvainD Feb 14 '14 at 8:47
3
\$\begingroup\$

I'm just going to review the Ntp class. As you'll see, there's plenty here for one answer.

  1. There's no documentation. What is this class supposed to do? What value does the time method return?

  2. There are some "tells" that you have come from a C background, and are not yet a fluent Python programmer: you have unnecessary semi-colons at the ends of lines, and unnecessary parentheses around the condition in if statements.

  3. The code is not portable to Python 3:

    >>> Ntp().time()
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "cr41537.py", line 20, in time
        client.sendto(self.msg, self.address)
    TypeError: 'str' does not support the buffer interface
    

    For portability to Python 3 you need msg to be a bytes object, not a str.

  4. Ntp is a "classic" or "old-style" class. This means that it doesn't support the super function, making it inconvenient to subclass. You should make new-style classes unless you have a good reason not to, by inheriting from object:

    class Ntp(object):
    
  5. It's not clear that there's any need for a class here. There's no persistent state, and just one method, so a function would work just as well and be simpler.

  6. In this line:

    self.msg = '\x1b'+47*'\0'
    

    it is not at all clear where the expression comes from. What does it mean, and how can I check that you got it right?

    By reference to the NTP standard in RFC 5905, it looks as though this decodes as version = 3 and mode = client. I would try to be much more explicit here as to how this packet is constructed, for example like this:

    # See section 7 of RFC 5905 for the structure of an NTP packet.
    fmt = '!BBbb3I4Q'
    version = 3
    mode = 3 # client mode
    msg = struct.pack(fmt, (version << 3) + mode, *[0]*10)
    

    Note that msg is now a bytes object as required by Python 3.

  7. You call socket.recvfrom to get the address of the socket sending the data, but then you never use it. So why not just use socket.recv?

  8. You are only going to use the first 48 bytes of the returned packet, so there is no point in setting the bufsize argument to socket.recv to be any larger.

  9. You decode the result like this:

    retval=struct.unpack("!12I",data);
    t = retval[10]
    

    but looking at RFC 5905, this gets you the top 32 bits of the 64-bit transmit timestamp (the integer part). Why are you throwing away the bottom 32 bits? Python's datetime module supports computation with microseconds, so it is worth having both. I would decode like it this:

    fmt = '!BBbb3I4Q'
    results = struct.unpack(fmt, data)
    tx_timestamp = results[10]
    

    and then if I need to convert to seconds I can divide by 232. Note also that I used the same format for decoding the received packet as I did for encoding the sent packet. This ensures consistency.

  10. In this code:

    # reference time (in seconds since 1900-01-01 00:00:00)
    TIME1970 = 2208988800 # 1970-01-01 00:00:00
    

    it is not clear where this value comes from. How would I check that you got it right? I would refer to figure 4 of RFC 5905 in the comment.

    You could also consider computing this constant explicitly using Python's datetime module, which would make its meaning clear:

    from datetime import datetime
    
    # Offset between NTP time and Unix epoch time in seconds.
    # See figure 4 of RFC 5905.
    TIME1970 = (datetime(1970, 1, 1) - datetime(1900, 1, 1)).total_seconds()
    
  11. There is no attempt to correct for the transmission delay as described in section 8 of RFC 5905. You need something like this:

    import time
    
    # Make sure DNS lookup time is not included in delay calculation.
    address = socket.getaddrinfo(host, port, AF_INET, SOCK_DGRAM)
    
    orig_time = time.time()
    client.sendto(msg, address[0][4])
    data = client.recv(48)
    dst_time = time.time()
    results = struct.unpack(fmt, data)
    recv_time = results[9] / 2.0**32
    tx_time = results[10] / 2.0**32
    
    # Estimate transmission delay: see section 8 of RFC 5905.
    delay = ((dst_time - orig_time) - (tx_time - recv_time)) / 2
    

    Note that the purpose of calling socket.getaddrinfo here (instead of passing (host, port) directly to client.sendto) is to avoid the time for the DNS lookup of host being included in the delay calculation.

  12. The time method returns the result in an inconvenient format (as the number of seconds since the epoch). It would be more generally useful to Python programmers to return a datetime.datetime object, like this:

    from datetime import datetime
    
    return datetime.utcfromtimestamp(tx_time - TIME1970 + delay)
    
\$\endgroup\$
  • \$\begingroup\$ Thank you for your generous feed back. I've incorporated all the changes you have mentioned here into my code. I should be looking at some code base on github to pick up in python and proper design styles. \$\endgroup\$ – raka Feb 14 '14 at 1:55
  • \$\begingroup\$ And yeah your guessed it right about my C background. \$\endgroup\$ – raka Feb 14 '14 at 2:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.