ImprSimple chat bot written in python

Question

I'd like to know if I can improve the performance of my recent bot, and maybe some design patterns too.

So far, it's warning a user if it's using some bad words (which are parsed from bad_words.txt) and it's entertaining the users by posting jokes from jokes.txt.

from selenium import webdriver
import time
import random
def login_to_rst(base_url, username, password):
 xpaths = {
 'usernameTxtField': "//input[@name='auth']",
 'passwordTxtField': "//input[@name='password']",
 'submitButton': "//*[@id='ipsLayout_mainArea']/div/div[2]/div/form/li[5]/div/button"
 }
 FIREFOX_DRIVER.get(base_url)
 FIREFOX_DRIVER.find_element_by_xpath(xpaths['usernameTxtField']).clear()
 FIREFOX_DRIVER.find_element_by_xpath(xpaths['usernameTxtField']).send_keys(username)
 FIREFOX_DRIVER.find_element_by_xpath(xpaths['passwordTxtField']).clear()
 FIREFOX_DRIVER.find_element_by_xpath(xpaths['passwordTxtField']).send_keys(password)
 FIREFOX_DRIVER.find_element_by_xpath(xpaths['submitButton']).click()
 FIREFOX_DRIVER.get(base_url)
def write_to_chat(chat_url):
 FIREFOX_DRIVER.get(chat_url)
 bad_words = [line.rstrip('\n') for line in open('bad_words.txt')]
 time.sleep(5)
 message = "Please use a decent language !"
 xpaths = {
 'textArea': "//*[@id='ipsTabs_elChatTabs_chatroom_panel']/div[1]/div[1]/textarea",
 'submitMessage': "//*[@id='ipsTabs_elChatTabs_chatroom_panel']/div[1]/div[1]/div[3]/button"
 }
 parsed_messages, rst_messages_list = [], []
 keep_running = True
 while keep_running:
 divs = FIREFOX_DRIVER.find_elements_by_xpath('//ul[@class="ipsList_reset cChatContainer"]/li/div')
 for div in divs:
 rst_messages_list = []
 if div.id not in parsed_messages:
 rst_messages_list.append(div.text)
 print(rst_messages_list)
 parsed_messages.append(div.id)
 for unique_message in rst_messages_list:
 for word in bad_words:
 if word in unique_message:
 FIREFOX_DRIVER.find_element_by_xpath(xpaths['textArea']).clear()
 FIREFOX_DRIVER.find_element_by_xpath(xpaths['textArea']).send_keys(message)
 FIREFOX_DRIVER.find_element_by_xpath(xpaths['submitMessage']).click()
 if '/message1' in unique_message or '/Message1' in unique_message:
 FIREFOX_DRIVER.find_element_by_xpath(xpaths['textArea']).clear()
 FIREFOX_DRIVER.find_element_by_xpath(xpaths['textArea']).send_keys("text to be sent")
 FIREFOX_DRIVER.find_element_by_xpath(xpaths['submitMessage']).click()
 if '/message2' in unique_message or '/Message2' in unique_message:
 FIREFOX_DRIVER.find_element_by_xpath(xpaths['textArea']).clear()
 FIREFOX_DRIVER.find_element_by_xpath(xpaths['textArea']).send_keys("2016 © me")
 FIREFOX_DRIVER.find_element_by_xpath(xpaths['submitMessage']).click()
 if '/message3' in unique_message:
 line = random.choice(open('jokes.txt').readlines())
 FIREFOX_DRIVER.find_element_by_xpath(xpaths['textArea']).clear()
 FIREFOX_DRIVER.find_element_by_xpath(xpaths['textArea']).send_keys(line)
 FIREFOX_DRIVER.find_element_by_xpath(xpaths['submitMessage']).click()
if __name__ == "__main__":
 FIREFOX_DRIVER = webdriver.Firefox()
 login_to_rst('https://link-login', 'user', 'pass')
 write_to_chat('https://link-final-url')

I accept any constructive critiques as long as they are followed by a rational explanation.

Answer 1

Avoid using open when you can. with is a keyword for using a context manager, which will automatically ensure that files are always properly closed, which you don't ensure when you just call open in a list comprehension.

with open('bad_words.txt') as f:
 bad_words = [line.rstrip('\n') for line in f]

keep_running is always True, so why not just use while True.

To reduce nesting levels, I'd flip the condition from this:

rst_messages_list = []
if div.id not in parsed_messages:
 rst_messages_list.append(div.text)

to this

rst_messages_list = []
if div.id in parsed_messages:
 continue
rst_messages_list.append(div.text)

continue moves onto the next iteration, so it is effectively the same as before except that your code isn't as deeply nested.

When testing unique_message, you should really make it lowercase for ease of testing. That way you can match regardless of case. Also when you're testing if any word in bad_words is in unique_message, there's a Python function that does that, called any:

for unique_message in rst_messages_list:
 unique_message = unique_message.lower()
 if any(word in unique_message for word in bad_words):

This does the same as you had before, but now the if and for statements are combined in one line. The one main difference, is that your old code would message the user repeatedly if they used multiple bad words, which I assume was not the intention.

You repeatedly call these lines:

FIREFOX_DRIVER.find_element_by_xpath(xpaths['textArea']).clear()
FIREFOX_DRIVER.find_element_by_xpath(xpaths['textArea']).send_keys(message)
FIREFOX_DRIVER.find_element_by_xpath(xpaths['submitMessage']).click()

Make this a function:

def send_message(message, xpaths):
 FIREFOX_DRIVER.find_element_by_xpath(xpaths['textArea']).clear()
 FIREFOX_DRIVER.find_element_by_xpath(xpaths['textArea']).send_keys(message)
 FIREFOX_DRIVER.find_element_by_xpath(xpaths['submitMessage']).click()

That reduces your code drastically:

if any(word in unique_message for word in bad_words):
 send_message(message, xpaths)
elif '/message1' in unique_message:
 send_message("text to be sent", xpaths)
elif '/message2' in unique_message:
 send_message("2016 © me", xpaths)
elif '/message3' in unique_message:
 line = random.choice(open('jokes.txt').readlines())
 send_message(line, xpaths)

Worth noting that since these are all if commands, you might send every one of these messages, for a message like "/message1 /message2 /message3 smeg" (assuming that smeg is on your list of bad_words). You should all but the first one elifs, so that they're mutually exclusive and only one (or none) will run.

Last point, unless calling webdriver.Firefox() is a lot of work for the script, I'd put that at the top instead of in if __name__ == "__main__", as it's easier to spot when it's near the top of the page, after imports. That's the convention for where constants should go.

FIREFOX_DRIVER = webdriver.Firefox()

• Grajdeanu Alex: thanks for the tips. They are great. One question though: if the second if will become elif shouldn't the next ifs to also become elifs? More, in send_message(message) method I think you forgot to add the xpaths, right ?
• Grajdeanu Alex: When you said about if div.id not in parsed_messages:, should I have all of those lines or I have to cut off the first 3 ones ?
• SuperBiasedMan: @Alex That's what I meant about elifs, I wasn't clear though so I edited that. You're right about xpaths. If xpaths was a global constant it wouldn't need to be explicitly passed, and it probably should be one. I'm not sure what you mean with that last point, you don't need to remove lines. I just showed a small snippet of how the script would look with that condition, to show the indentation of those lines.
• Grajdeanu Alex: I was talking about: To reduce nesting levels, I'd flip the condition from this: and I was expecting a piece of code after that statement, then something like to this: and some other code after. Now, I'm confused because I don't know what to keep in that block ^__^
• SuperBiasedMan: @Alex Oh woops. I left out my "to this", is it clearer in the edited version?