2
\$\begingroup\$

A Leap Card is essentially a travel card for Dublin, Ireland that can be used to pay for a bus or tram fare. It has to be topped up and I often forget how much I have left on it.

To get familiar with Selenium, I wrote some code that gets the balance for me. I haven't worked with this framework before, so I'd like to know if there's room for improvement here.

import json

from selenium import webdriver


def get_credentials():
    """Get user credentials from a JSON file"""
    with open('auth.json', 'r') as f:
        auth = json.load(f)
        username = auth['username']
        password = auth['password']
    return username, password


class LeapHandler:
    """Class to handle any interactions with Leap Card website"""

    def __init__(self):
        self.driver = webdriver.Chrome()
        self.driver.get('https://www.leapcard.ie/en/login.aspx')
        self.login()

    def get_login_elements(self):
        """Find and return elements necessary to log in"""
        username_id = 'ContentPlaceHolder1_UserName'
        password_id = 'ContentPlaceHolder1_Password'
        login_id = 'ContentPlaceHolder1_btnlogin'
        username_field = self.driver.find_element_by_id(username_id)
        password_field = self.driver.find_element_by_id(password_id)
        login_button = self.driver.find_element_by_id(login_id)
        return username_field, password_field, login_button

    def login(self):
        """Login into user account"""
        username_field, password_field, login = self.get_login_elements()
        username, password = get_credentials()
        username_field.send_keys(username)
        password_field.send_keys(password)
        login.click()

    def get_balance(self):
        """Return current Leap Card Balance"""
        balance = self.driver.find_element_by_css_selector('div.pull-left').text
        return balance

if __name__ == '__main__':
    lh = LeapHandler()
    balance = lh.get_balance()
    print(f'LepaCard balance is €{balance}')
\$\endgroup\$
1
\$\begingroup\$

Your code is actually quite clean and readable, good job. Here are some of the things I would consider changing:

  • you can simplify get_credentials() a bit, using the tuple packing/unpacking:

    def get_credentials():
        """Get user credentials from a JSON file."""
        with open('auth.json', 'r') as f:
            auth = json.load(f)
            return auth['username'], auth['password']
    
  • end your docstrings with a dot as per PEP8
  • if the page manipulation selenium code is going to continue grow, you should definitely consider switching to using Page Object pattern building abstractions around the webpages you are working with. But, for this kind of problem, it is perfectly fine to not use page objects
  • div.pull-left does not look like a good quality locator. It may work now, but this locator is fragile because it is design-specific (pull-left is a bootstrap class used for page layout). See if this element has an id or a data-oriented class or a data-oriented attribute.
\$\endgroup\$
  • \$\begingroup\$ Using the div.pull-left locator is a workaround, as I couldn't get the text any other way. Here is the snippet from page source: pastebin.com/Z3Kec3zD \$\endgroup\$ – Lukasz Salitra Jul 31 '17 at 21:24
  • 1
    \$\begingroup\$ @LukaszSalitra right, not the easiest markup to deal with. I would probably use a preceding label with, for example, an XPath - something like: //div[contains(label/@id, "TravelCreditBalance")]/following-sibling::div/div - not the simplest expression though. Looking at that, your workaround looks better, doesn't it? :) \$\endgroup\$ – alecxe Jul 31 '17 at 21:32
  • \$\begingroup\$ Objectively, it does! :) But as you said, it is fragile and perhaps not the best. I wanted to get going with XPath either way, so I'm sure that will give me a right idea! Thanks for the review, thoughtful as always. \$\endgroup\$ – Lukasz Salitra Jul 31 '17 at 21:39

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.