Private client-server chat

Question

I try to create a private chat with Python but I don't know best practice.

It's only for study purpose, I'd like understand the logic behind a simple private chat with socket and the best mode to implement it with no framework.

For now, It's not important to check for example if username has> or other, only socket procedure and send messages to specific client.

This is my code for client:

import socket
from thread import *
from time import sleep
s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
s.connect(('127.0.0.1', 8889))
u = raw_input('Username: ')
print 'To send: recipient>message'
def se(s):
 while 1:
 s.send(u + '>' + raw_input())
 print s.recv(1024)
def re(s):
 while 1:
 s.send(u + '>show>')
 r = s.recv(1024)
 if r != 'No messages':
 print r
 sleep(0.05)
start_new_thread(se ,(s,))
start_new_thread(re ,(s,))
while 1:
 pass

and server:

import socket
from thread import *
import string
s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
s.bind(('127.0.0.1', 8889))
s.listen(10)
c = {}
def clientthread(conn):
 while 1:
 data = conn.recv(1024)
 e = data.split('>')
 if len(e) == 3:
 if e[1] == 'show':
 try:
 m = ''
 for i in range(0, len(c[e[0]])):
 m = m + c[e[0]][i]
 if i != (len(c[e[0]])-1):
 m = m + '\n'
 if m == '':
 data = 'No messages'
 else:
 data = m
 del c[e[0]]
 except:
 data = 'No messages'
 else:
 try:
 c[e[1]]
 except:
 c[e[1]] = []
 c[e[1]].append(e[0]+'>'+e[2])
 data = 'Ok'
 else:
 data = 'Error'
 conn.send(data)
 conn.close()
while 1:
 conn, addr = s.accept()
 start_new_thread(clientthread ,(conn,))
s.close()

With this code every user can send and recive messages. For example if there are 2 users (2 clients with server active): max and paul; if max writes paul>hello paul recives max>hello.

Program uses dictionary and lists for send message to particular client. I don't know if this way is correct.

Answer 1

1. First off, don't use 1 over True in while loops. While 1 works, using True is much clearer.
2. Secondly, you have inconsistent indentation. While in Python 2.x, this doesn't affect much, Python 3.x will throw an error.
3. Never use try-except without a specific error to catch. There could easily be an error that goes un-noticed.
4. Most of your names are okay, but there are some like c, or e that should be clearer. Variable and function names should also be in snake_case, and classes should be in PascalCase. Names like conn should be renamed to something else, like connection, for example.
5. You need more blank lines. There should be two blank lines in between each code block/function/class on the top level.
6. Finally, you should avoid wildcard imports like this: from thread import *. You should either import the entire module, or any classes/functions/variables that you need.

Here's Python's official style guide, PEP8

• Ethan Bierlein: @Artux87 No problem! If the answer helped, you can click the check mark to accept it.
• janos: re and se are pretty very bad names too

Answer 2

Naming

@Ethan was too kind to you about variable names. This code is barely readable, due to all the single-letter variable names in the most important controlling logic of the program. Focusing on write-time convenience is a false economy: code is read far more often than written. It's very well worth writing code in a way that's easy to read.

Readability

Make the code easier to read by introducing local variables that give name to otherwise nameless elements that hurt readability. Consider for example this piece:

e = data.split('>')
if len(e) == 3:
 if e[1] == 'show':

It looks like e[1] is some kind of command, e[0] might be sender and e[2] receiver, so give them names:

sender, command, receiver = e

The rest of the code becomes so much easier to read.

Use try-except to handle exceptions

All the try-except blocks are misused. You should use try-except for handling abnormal situations. In this program, the exceptions get raised under perfectly normal operations. Practically you're using them for checking dictionary keys. The recommended way to check dictionary keys is with the in operator.

For example, instead of this:

try:
 c[e[1]]
except:
 c[e[1]] = []

Write like this:

if e[1] not in c:
 c[e[1]] = []

This is shorter, and the c[e[1]] in the original try block was wrong for another reason: it's a statement with no effect, only the side effect of raising a KeyError. It's a bad practice to do things for their side effects.

More natural and improved iteration over lists

Instead of this:

for i in range(0, len(c[e[0]])):
 m = m + c[e[0]][i]
 if i != (len(c[e[0]])-1):
 m = m + '\n'
 if m == '':
 data = 'No messages'
 else:
 data = m

Consider this more idiomatic alternative:

for i, item in enumerate(c[e[0]]):
 m += item
 if i != (len(c[e[0]])-1):
 m += '\n'
 if m:
 data = m
 else:
 data = 'No messages'

A sneaked in a few other improvements:

• Thanks to the enumerate, c[e[0]][i] can be replaced with something simpler and more intuitive. I called it "item" because I don't fully understand your code. Think of a better name.
• Instead of x = x + ... you can writer simpler x += ...
• I flipped the last if-else, for two reasons:
  • It's shorter to write this way than != ''
  • It's generally good to have the normal case as the first