7
\$\begingroup\$

I have been learning Python and was trying to make a chat system which can be run by commands. This is my first attempt to write the code. Does this make sense or is it wrong usage of classes?

There is a User class which will contain the user of the chat. The Message class is used to send messages and count their length.

class User:
   def __init__(self):
        self.users = []

   def add_remove_user(self,input):
        command, name = input[:1], input[1:]

        if command == "+":
            if not name in self.users:
              self.users.append(name)
        elif command == "-":
            if name in self.users:
                self.users.remove(name)


class Message:
    def __init__(self,user):
        self._messages=[]
        self._user=user

    def __parseMessage__(self,message):
        parsedMessage=tuple(message.split(":"))
        return parsedMessage


    def send_message(self,inputMessage):
        user,message = self.__parseMessage__(inputMessage)
        if user in self._user.users:
            self._messages.append(message)

    def sent_messages_count(self):
        count=0
        for message in self._messages:
            count += len(message)

        return count

class MessageClient:
    def __init__(self):
        self.user=User()
        self.message=Message(self.user)

    def send(self,inputMessage):
        if inputMessage[0] == "+" or inputMessage[0] == "-":
            self.user.add_remove_user(inputMessage)
        else:
            self.message.send_message(inputMessage)

    def sent_message_length(self):
        return self.message.sent_messages_count()
\$\endgroup\$
  • \$\begingroup\$ Which version of Python, and is there some test to run it with? \$\endgroup\$ – jonrsharpe Jul 23 '14 at 12:39
3
\$\begingroup\$

A few comments regarding style (mainly based on PEP8):

  • Use for spaces for each level of indentation
  • Use a space around binary operators like =
  • Use a space after a , for an argument
  • Do not use the camelCase naming style: parsedMessage -> parsed_message, inputMessage -> input_message.
  • Try not to use built-in function names (input)
  • For private methods names use a single underscore (there's a use case for double leading underscore in complex inheritance, but that's not the case here)
  • Use x not in y instead of not x in y
  • Write docstrings (PEP257)

Some other comments:

  • Why have an add_remove_user method? Having both add_user and remove_user methods would be more readable.
  • For the users you probably want to use a set
  • Be careful with length message.split(':'). You probably want message.split(':', 1) to avoid problems with messages that contain colons in them.
  • sent_message_length and sent_message_count don't seem to implement what I would expect from their name
  • Use and instead of nesting if statements. For example, instead of this:

    if command == "+":
        if not name in self.users:
            self.users.append(name)
    

    write this:

    if command == "+" and name not in self.users:
        self.users.append(name)
    
\$\endgroup\$
  • \$\begingroup\$ that is some good point , how about class structure. should user class do the parsing of input message or it should be part of some other class like inputvalidator \$\endgroup\$ – paritosh Jul 24 '14 at 5:19
  • 1
    \$\begingroup\$ I think there's still some work to be done in the code. In my opinion the classic design is having a client class, a server class, a channel class and a message class. The client objects connect to the server, join one or more channels and send messages through a channel. The server object forwards the messages to all clients connected to a channel. The channel objects might be just a list of users or maybe act as a message queue of pending messages. The message objects might be just containers or do some encoding/decoding. \$\endgroup\$ – jcollado Jul 24 '14 at 7:57

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.