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

For a class, I was given an assignment to code a simple TCP connection between a server and a client. Once the TCP handshake is done, the client sends inquiries to the server. It's a 2 second conversation, if that. I need some feedback on my code. Please keep in mind that I am new to Python. The prof kinda threw the class to the wolves on this.

This is the server code:

import socket
import time
import sys
#from thread import *

def Main():
        s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
        host = socket.gethostname()
        port = 8008


        s.bind((host, port))


        print ("Port, that's a wine right? " + str(port))

        s.listen(1)
        c, addr = s.accept()
        print ("200 SRP version 1.0 ready")
        print ("Connection from: " + str(addr))
        while True:
            data = c.recv(1024)
            data = str(data).upper()
            print ("sending: " + str(data))
            c.send(data)            
            print ("from connected user: " + str(data))
            print ("Welcome, would you like a jelly baby?")

            if not data:
                        break
            if True:
                if data.startswith('HELO'):
                    try:
                        c.sendall("210 HELO " +  host + " This is Dalek High Command. You will obey all orders without question!")
                    except:
                        c.sendall("510 You are unauthorized! You will be exterminated!")
                    break
                elif data.startswith('REQTIME'):
                    try:
                        c.sendall ("220 I obey!  The time is " + time.strftime("%H%:M%:S"))
                    except:
                        c.sendall ("520 The time is unavailable!  Exterminate!")
                        break
                elif data.startswith('REQDATE'):
                        try:
                        c.sendall ("230 I obey!  The date is " + time.strftime("%Y/%m/%d"))
                    except:
                        c.sendall ("530 The date is unavailable!  Exterminate!")
                        break
                elif data.startswith('ECHO'):
                    try:
                        data = str(data).upper()
                        c.sendall ("240 Data received! " + data)
                    except:
                        c.sendall ("540 ECHO!  I cannot obey!  Assist!  Assist!  Cannot comply!  Exterminate!")
                        break
                elif data.startswith('REQIP'):
                    try:
                        c.sendall ("250 IP has been identified! " + host)
                    except:
                        c.sendall ("550 Alert!  I cannot obey! Your IP cannot be retrieved!")
                        break
                elif data.startswith('BYE'):
                    try:
                        c.sendall ("600 Alonzy! Bowties are cool!  See you later!")
                    except:
                        break
sys.exit()
c.close()

This is the client:

import socket

def Main():
        host = ''
        port = 8008

        s = socket.socket()
        s.connect((host, port))

        message = s.recv(1024)
        print('Received from server: ' + str(data))


        for x in range(0, 5):
            if x == 0:
                data = 'helo'
                s.send(data)
            elif x == 1: 
                data = 'reqtime'
            elif x == 2:
                data = 'reqdate'
            elif x == 3:    
                data = 'echo'
            elif x == 4:
                data = 'reqid'
            elif x == 5:
                data = 'bye'

        s.send(data)
        print(str(data))
        x = x + 1
share|improve this question
    
Welcome to Code! Good job on your first question. – SirPython Mar 30 at 1:07
    
So does it look okay, @SirPython? – Shannon Mar 30 at 1:09
    
Looks fine to me. – SirPython Mar 30 at 1:12
    
I've tested them locally and I keep getting an indentation error on the server on line 46. I'm using Notepad++ and all my indents look fine. I use tabs instead of the Spacebar. c.sendall ("230 I obey! The date is " + time.strftime("%Y/%m/%d")) – Shannon Mar 30 at 1:36
    
@Shannon: You need to un-indent the line above that. – zondo Mar 30 at 2:06

I would recommend that you read PEP 8, the Python style guide.

PEP 8 says:

Use 4 spaces per indentation level.

You use four spaces in some places, but in others you use eight. At the very least you should be consistent, but it is best if you consistently use four spaces. In fact, looking at your code, your indentation is completely shot. I spy with my little eye an IndentationError in the elif data.startswith('REQDATE'): block. Your break statements in those if blocks are not consistent either.

print("sending: " + str(data))

One line up you defined data as a string. Python objects don't just drift into new types. They don't change unless you change them. You don't need the str part here. Just do this:

print("sending: " + data)

You convert data to a string in several other places, but there also it is useless. That's like translating a German sentence into English and then translating that sentence into English every time I want to use it. What is the purpose of translating English into English?

if True:

True is always True, so that if block will always be executed. Why do you use an if block if you always want to execute what is inside?

if data.startswith('HELO'):
    try:
        c.sendall(...)
    except:
        c.sendall(...)
    break
elif data.startswith('REQTIME'):
    try:
        c.sendall(...)
    except:
        c.sendall(...)
        break
...

You have some duplicate code there. That is, it would be duplicate if you were consistent with what you wanted. There are two things that change in these: The string given to the first c.sendall() and the string given to the second c.sendall(). I would create a dictionary:

messages = {
    'HELO': (
        "210 HELO " + host + " This is Dalek High Command.  You will obey all orders without question!",
        "510 You are unauthorized! You will be exterminated!"),
    'REQTIME': (
        "220 I obey!  The time is " + time.strftime("%H%:M%:S"),
        "520 The time is unavailable!  Exterminate!"),
    'REQDATE': (
        "230 I obey!  The date is " + time.strftime("%Y/%m/%d"),
        "530 The date is unavailable!  Exterminate!"),
    'ECHO': (
        "240 Data received! " + data.upper(),
        "540 ECHO!  I cannot obey!  Assist!  Assist!  Cannot comply!  Exterminate!"),
    'REQIP': (
        "250 IP has been identified! " + host,
        "550 Alert!  I cannot obey! Your IP cannot be retrieved!"),
}

for key, value in messages.items():
    if data.startswith(key):
        try:
            c.sendall(value[0])
        except:
            c.sendall(value[1])
        break
else:
    if data.startswith('BYE'):
        try:
            c.sendall ("600 Alonzy! Bowties are cool!  See you later!")
        except:
            break
sys.exit()
c.close()

You exit the program and then expect to be able to close c? Perhaps that's a debugging call that you forgot to take out, but sys.exit() is unnecessary here.

for x in range(0, 5):
    if x == 0:
        data = 'helo'
        s.send(data)
    elif x == 1: 
        data = 'reqtime'
    elif x == 2:
        data = 'reqdate'
    elif x == 3:    
        data = 'echo'
    elif x == 4:
        data = 'reqid'
    elif x == 5:
        data = 'bye'

s.send(data)
print(str(data))
x = x + 1

I'm guessing that the s.send(data) and below lines were meant to be inside of the for loop. If they were, there is a much simpler way to do this:

for data in ("helo", "reqtime", "reqdate", "echo", "reqid", "bye"):
    s.send(data)
    print(data)
share|improve this answer

The sequence

    try:
        sendall(...)
    except:
        sendall(...)

doesn't look right. The except clause is executed when sendall threw the exception. The exception is most likely caused to the network problem, so there is a big chance that the second sendall will also fail. Even more important is misattribution. For example, in

        try:
            c.sendall("210 HELO " +  host + " This is Dalek High Command. You will obey all orders without question!")
        except:
            c.sendall("510 You are unauthorized! You will be exterminated!")

the exception is raised by your attempt to send, yet you are going to exterminate the user (who apparently did nothing wrong).

I can also make an educated guess that the server is not supposed to execute any command until the client sends helo.

share|improve this answer
    
Do I really need to explain the 'exterminate' thing? – Shannon Apr 11 at 18:20

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.