3
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code! Good job on your first question. \$\endgroup\$ – SirPython Mar 30 '16 at 1:07
  • \$\begingroup\$ So does it look okay, @SirPython? \$\endgroup\$ – Shannon Mar 30 '16 at 1:09
  • \$\begingroup\$ Looks fine to me. \$\endgroup\$ – SirPython Mar 30 '16 at 1:12
  • \$\begingroup\$ 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")) \$\endgroup\$ – Shannon Mar 30 '16 at 1:36
  • \$\begingroup\$ @Shannon: You need to un-indent the line above that. \$\endgroup\$ – zondo Mar 30 '16 at 2:06
2
\$\begingroup\$

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)
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Do I really need to explain the 'exterminate' thing? \$\endgroup\$ – Shannon Apr 11 '16 at 18:20

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.