Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

Here is my code for a Python script for a peer to peer chat system on a network using sockets. I know there are some things that could be changed so there would be less repetition in the code and that some variables could be global, but what I am asking is if there are any issues with the code's functionality; it seems to work properly on my home network but will only connect (not receive messages) on my school network. Because it establishes a connection there, it looks like the port is not being blocked by the internal firewall.

#!usr/bin/env python

import socket
import threading
import select
import time
import datetime

def main():

    class Chat_Server(threading.Thread):
            def __init__(self):
                threading.Thread.__init__(self)
                self.running = 1
                self.conn = None
                self.addr = None
            def run(self):
                HOST = ''
                PORT = 8080
                s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
                s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
                s.bind((HOST,PORT))
                s.listen(1)
                self.conn, self.addr = s.accept()
                # Select loop for listen
                while self.running == True:
                    inputready,outputready,exceptready \
                      = select.select ([self.conn],[self.conn],[])
                    for input_item in inputready:
                        # Handle sockets
                        message = self.conn.recv(1024)
                        if message:
                            print "Daniel: " + message + ' (' + datetime.datetime.now().strftime('%H:%M:%S') + ')'
                        else:
                            break
                    time.sleep(0)
            def kill(self):
                self.running = 0

    class Chat_Client(threading.Thread):
            def __init__(self):
                threading.Thread.__init__(self)
                self.host = None
                self.sock = None
                self.running = 1
            def run(self):
                PORT = 8080
                self.sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
                self.sock.connect((self.host, PORT))
                # Select loop for listen
                while self.running == True:
                    inputready,outputready,exceptready \
                      = select.select ([self.sock],[self.sock],[])
                    for input_item in inputready:
                        # Handle sockets
                        message = self.sock.recv(1024)
                        if message:
                            print "Daniel: " + message + ' (' + datetime.datetime.now().strftime('%H:%M:%S') + ')'
                        else:
                            break
                    time.sleep(0)
            def kill(self):
                self.running = 0

    class Text_Input(threading.Thread):
            def __init__(self):
                threading.Thread.__init__(self)
                self.running = 1
            def run(self):
                while self.running == True:
                  text = raw_input('')
                  try:
                      chat_client.sock.sendall(text)
                  except:
                      Exception
                  try:
                      chat_server.conn.sendall(text)
                  except:
                      Exception
                  time.sleep(0)
            def kill(self):
                self.running = 0

    # Prompt, object instantiation, and threads start here.

    ip_addr = raw_input('Type IP address or press enter: ')

    if ip_addr == '':
        chat_server = Chat_Server()
        chat_client = Chat_Client()
        chat_server.start()
        text_input = Text_Input()
        text_input.start()

    else:
        chat_server = Chat_Server()
        chat_client = Chat_Client()
        chat_client.host = ip_addr
        text_input = Text_Input()
        chat_client.start()
        text_input.start()

if __name__ == "__main__":
    main()
share|improve this question
    
Welcome to Code Review! There are currently closing votes on your question since you mention problem with it. Note that we do not fix broken code here. As your problem seems like a network problem ( I don't know python to conclude if your code has a problem), I would suggest you edit your question to remove the problem part and just ask for a regular review. – Marc-Andre Oct 3 '14 at 14:25
1  
@Marc-Andre I'm voting to leave open - firewall issues aren't issues with the code. OP also says it gets the "works on my machine" certificate, I take it as working code ;) – Mat's Mug Oct 4 '14 at 1:34

Regular review.

  • main is bloated. All the class definitions do net belong there.

  • Creating a Chat_Client instance in a server branch (and vice versa) looks strange. My guess it is a result of a Text_Input design, which assumes that both server and client exist. This is of course wrong. A Text_Input instance should have just one "sink" where the text input would go:

    class Text_Input(threading.Thread):
            def __init__(self, sink):
                threading.Thread.__init__(self)
                self.sink = sink
                self.running = 1
            def run(self):
                while self.running == True:
                  text = raw_input('')
                  self.sink.send(text)
    

    and server initialization becomes

    chat_server = Chat_Server()
    text_input = TextInput(chat_server)
    

    Same goes for a client. Both server and client should implement send method.

  • Network initialization should be moved into constructors. After that, run and send method (same for client and server) should be factored out into a separate Chatter class which client and server inherit from.

  • What is the purpose of time.sleep(0)? If the system misbehaves without it, then there is a bug to be fixed, not masked. Hint: you should select for outputready only when there is a message to be sent.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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