-1
\$\begingroup\$

So currently I have this basic little chat bot that can read commands and can timeout users if their message contains a banned word or phrase. I was wondering how I can improve on this bot to be able to !add "word" to the set of banned words and overall general flaw improvements.

import string
from Read import getUser, getMessage
from Socket import openSocket, sendMessage
from Initialize import joinRoom

s = openSocket()
joinRoom(s)
readbuffer = ""
banned_set = {"badword1", "badword2"}
while True:
        readbuffer = readbuffer + s.recv(1024)
        temp = string.split(readbuffer, "\n")
        readbuffer = temp.pop()

        for line in temp:
            print(line)
            if "PING" in line:
                s.send(line.replace("PING", "PONG"))
                break
            user = getUser(line)
            message = getMessage(line)
            print user + " typed :" + message
            if not banned_set.isdisjoint(message.lower().split()):
                sendMessage(s, "/timeout " + user)
                break
            if "!guitars" in message:
                sendMessage(s, "Ibanez RG920QM Premium")
                break
\$\endgroup\$
1
  • \$\begingroup\$ Adding features is off-topic for Code Review, so I think your !add "word" idea may not be realised here. \$\endgroup\$ – Tersosauros Apr 2 '16 at 11:37
1
\$\begingroup\$

In addition to the issues @zondo has pointed out (i.e. PEP 8, some better operators, and the string features), I would also like to point out a few things.


1) Variable names
Variables names such as temp are to be avoided. A much better name for this variable would be something like lines, messages, stack, messageStack^, etc.

^ Note:  non PEP 8 camelCasing used to be consistent with existing code as posted. Obviously you would make this message_stack when fixing that issue.



2) Don't PONG everybody!
In your code, it should be noted, that lines 17 - 19 inclusive (shown below for brevity) introduce some (probably?) undesired behaviour...

        if "PING" in line:
            s.send(line.replace("PING", "PONG"))
            break

Consider that a user in the chat says "PING". Your bot will replace it with PONG and send the message back to the room.

This would be particularly bad given that this if-statement occurs before the banned words checking code (and break's out of the loop). Users can now use bad words to their heart's content, provided they include the word "PING" (in uppercase) in their message! Furthermore, the bot will repeat these bad words back to the room!! (This is how security bugs get created)

Note, if you do end up implementing an !add command to insert items into banned_set, PLEASE ensure you have successfully protected your adding code from injection!



3) Decide your case-consistency and stick with it.
On line 23 you include a call to message.lower() (the result of which is not stored anywhere). Then on line 26 your compare message to a lower-case command string ("!guitars"). Do you want "!Guitars" to work just like "!guitars"? If so, you may want to make message lowercase before you split it (as you're already doing for the bad-words check).

Furthermore, with your current logic the message "I've just added the !guitars command to my bot" will trigger the same response as just saying "!guitars". This is because your current logic (using the in operator,) disregards the position of the command string within the message.

\$\endgroup\$
3
\$\begingroup\$

I'm guessing that Read, Socket, and Initialize are modules you wrote. The functions defined in there do not follow PEP 8 naming conventions. PEP 8 says to use snake_case, not mixedCase.

Your indentation also does not follow PEP 8. You should use four-space indentation, not eight-space. At the very least, you should be consistent.

readbuffer = readbuffer + s.recv(1024)

Re-defining something to be itself plus something else is so common that many languages have the += operator. You can use it here:

readbuffer += s.recv(1024)

Python also has *=, /=, //=, ^=, etc.

temp = string.split(readbuffer, "\n")

There is a better way to split each line:

temp = readbuffer.splitlines()

Since that line was the only place you used the string module, you can remove that import. You didn't need it anyway, though, because str has the .split() method.

print user + " typed :" + message

Instead of concatenation, use a format string:

print "{0} typed: {1}".format(user, message)
\$\endgroup\$

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.