3
\$\begingroup\$

In my program, I prompt the user to enter a range of cities (in a spreadsheet), and then a range of review scores, and then make some decisions based on what the user has entered. Since the handling of the both user inputs (both ranges) is common to each, I have combined both into the single function getRanges() below. It works well, but I get the sense that can be improved (shortened, and with fewer if/else's). In the absence of a switch statement in Python, what are some neater approaches here?

# -*- coding: utf-8 -*-
from selenium import webdriver
from selenium.webdriver.common.by import By
from selenium.webdriver.common.keys import Keys
from selenium.webdriver.support.ui import Select
from selenium.common.exceptions import NoSuchElementException
from selenium.common.exceptions import NoAlertPresentException
from selenium.common.exceptions import ElementNotVisibleException
from selenium.common.exceptions import WebDriverException
import unittest, time, re
from functions import *
from collections import OrderedDict
import csv
from openpyxl import load_workbook
from easygui import enterbox, msgbox
import traceback

class bot(unittest.TestCase):
    def setUp(self):
        self.wb = load_workbook("Spreadsheet.xlsx", data_only=True)
        self.cities = []
        self.getRanges('cities')
        self.getRanges('reviews')
        print 'Running...'

        try: 
            self.driver = webdriver.Firefox()
        except WebDriverException, e:
            print "Unable to load profile, retrying"
            try: 
                self.driver = webdriver.Firefox()
            except WebDriverException, e:
                print "Unable to load profile, retrying"
                self.driver = webdriver.Firefox()
        self.driver.implicitly_wait(30)
        self.base_url = "https://www.google.com/ncr"
        self.verificationErrors = []
        self.accept_next_alert = True
        self.reviewPattern = re.compile(ur'\d+')
        self.niche = "plumbers+"

    def getRanges(self, citiesOrReviews):
        self.LowerLimit = enterbox("Enter a lower limit for %s, or 'all'" % citiesOrReviews, 'Range')
        if self.LowerLimit.lower().strip("'") == "all":
            self.reviewRange = False
            if citiesOrReviews.lower() == 'cities':
                for row in self.wb.active.rows:                 
                    self.cities.append(row[0].value)
        elif self.LowerLimit.isdigit():
            if citiesOrReviews.lower() == 'cities':
                if int(self.LowerLimit) > len(self.wb.active.rows):
                    msgbox("The lower limit you've entered is higher than the number of cities in the spreadsheet. Let's retry.", 'Error')
                    self.getRanges(citiesOrReviews)
            self.UpperLimit = enterbox("Enter an upper limit for %s:" % citiesOrReviews, "Range")
            if self.UpperLimit.isdigit():
                if citiesOrReviews.lower() == 'cities':
                    if int(self.UpperLimit) > len(self.wb.active.rows):
                        msgbox("The upper limit you've entered is higher than the number of cities in the spreadsheet. Let's retry.", "Error")
                        self.getRanges(citiesOrReviews)
                    if self.UpperLimit <= self.LowerLimit:
                        msgbox("The upper and lower limits are identical, or the upper limit is less than the lower. Let's retry.", "Error")
                        self.getRanges(citiesOrReviews)
                    else:
                        if citiesOrReviews.lower() == 'cities':
                            for row in self.wb.active.rows[int(self.LowerLimit) - 1:int(self.UpperLimit)]:
                                self.cities.append(row[0].value)
                elif citiesOrReviews.lower() == 'reviews':
                    self.reviewRange = [int(self.LowerLimit) - 1, int(self.UpperLimit)]
        else:
            msgbox("The value entered must either be 'all', or a number. Let's retry.", "Error") 
            self.getRanges(citiesOrReviews)
\$\endgroup\$
2
  • \$\begingroup\$ Have you considered doing a bit of research on Pythonic equivalents of switch? For example: stackoverflow.com/questions/60208/… \$\endgroup\$
    – jonrsharpe
    Commented Aug 22, 2015 at 21:31
  • \$\begingroup\$ @jonrsharpe Thanks yes I'm familiar with that post, but really I'm looking to reduce the number of if/else's, and I don't think (because of how I have less-than-optimally sequenced the flow of the function) that utilizing elif is really going to help me out here. \$\endgroup\$
    – Pyderman
    Commented Aug 22, 2015 at 23:52

1 Answer 1

3
\$\begingroup\$

I will only comment on the "getting rid of the repeating ifs part". A way to avoid writing the same condition in different places is to move the check entirely into a separate piece of code. Some may refer to this as "state pattern". Below is an example:

class StateExample(object):

    class State(object):

        def __init__(self, example):
            self.example = example

        def __str__(self):
            return self.__class__.__name__

    class Cities(State):

        def query_limits(self):
            self.example.do_one_thing(self)

    class Reviews(State):

        def query_limits(self):
            self.example.do_something_else(self)

    def __init__(self):
        self.state_factories = {
            'cities': self.Cities,
            'reviews': self.Reviews
        }

    def do_one_thing(self, state):
        print 'doing one thing with %s' % state

    def do_something_else(self, state):
        print 'doing something else with %s' % state

    def interact(self, city_or_review):
        self.state = self.state_factories[city_or_review](self)

    def interact_more(self):
        self.state.query_limits()

And an example usage, to illustrate the usage:

>>> example = StateExample()
>>> example.interact('cities')
>>> example.interact_more()
doing one thing with Cities
>>> example.interact('reviews')
>>> example.interact_more()
doing something else with Reviews
>>> 

You may or may not need all of the machinery here. The important part is to notice how this would be equivalent to testing repeatedly for the same condition inside interact and interact_more, which in the example above happens only once.

You can read more about this technique here: https://en.wikipedia.org/wiki/State_pattern

As an aside: it is not necessary to use classes and objects to achieve the same goal, but I want to keep this answer short.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.