2
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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()
\$\endgroup\$
  • \$\begingroup\$ 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 ? \$\endgroup\$ – Grajdeanu Alex. Feb 16 '16 at 11:09
  • \$\begingroup\$ 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 ? \$\endgroup\$ – Grajdeanu Alex. Feb 16 '16 at 11:26
  • \$\begingroup\$ @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. \$\endgroup\$ – SuperBiasedMan Feb 16 '16 at 11:41
  • \$\begingroup\$ 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 ^__^ \$\endgroup\$ – Grajdeanu Alex. Feb 16 '16 at 11:48
  • \$\begingroup\$ @Alex Oh woops. I left out my "to this", is it clearer in the edited version? \$\endgroup\$ – SuperBiasedMan Feb 16 '16 at 11:51

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.