2
\$\begingroup\$

Using Python's Beautiful Soup, I analyze a number of html files (internally: soup). They are all a little different and my target comes in four different tags - or not at all. In two cases, my target may not be there but the tags may exists. In this case I find a .

Here I got a very ugly code to check for them:

#1: in <div class = "section ack">
if len(soup.find_all("div", {"class":"section ack"})) > 0:
    thanktag = soup.find("div", {"class":"section ack"})
#2: in <div class = "section fn-con">
elif len(soup.find_all("li", {"class":"fn-con"})) > 0:
    thanktag = soup.find("li", {"class":"fn-con"})
#3: in next-to-last <li class = "fn">
elif len(soup.find_all("li", {"class":"fn"})) > 1:
    thanktag = soup.find_all("li", {"class":"fn"})[-2]
#4: in first <li class = "fn">
    if "↵" in clean_string(thanktag.text):
        thanktag = soup.find_all("li", {"class":"fn"})[0]
#5: doesn't exist at all
if 'thanktag' in globals() and not "↵" in clean_string(thanktag.text):
    do_something()

For example, my code doesn't adhere to DRY. How could I make the code more pythonic/less redundant?

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

I'm interested to hear other approaches and any suggestions to mine. Here are two that I came up with:

Without a Helper Class

thanktag_queries = [
    ("div", {"class":"section ack"}, 0),
    ("li", {"class":"fn-con"}, 0),
    ("li", {"class":"fn"}, 1)
]

thanktag = None
for i in xrange(len(thanktag_queries)):
    thanktag_query = thanktag_queries[i]
    soup_result = soup.find_all(thanktag_query[0], thanktag_query[1])
    if len(soup_result) > thanktag_query[2]:
        thanktag = soup_result[0]

        if i == len(thanktag_queries)-1:
            if not contains_carriage_return(thanktag.text):
                thanktag = soup_result[-2]

        break
if thanktag != None and not contains_carriage_return(thanktag.text):
    do_something()

def contains_carriage_return(text):
    return "↵" in clean_string(text)

With a Helper Class

class ThanktagQuery:
    soup_result = None

    def __init__(self, tag, soup_options, threshold):
        self.tag = tag
        self.soup_options = soup_options
        self.threshold = threshold

    def execute_query():
        self.soup_result = soup.find_all(self.tag, self.soup_options)

    def contains_thanktag():
        try:
            return len(self.soup_result) > self.threshold
        except TypeError as exc:
            raise RuntimeError("Query has not been run yet for this instance.")

thanktag_queries = [
    ThanktagQuery("div", {"class":"section ack"}, 0),
    ThanktagQuery("li", {"class":"fn-con"}, 0),
    ThanktagQuery("li", {"class":"fn"}, 1))
]

thanktag = None
for i in xrange(len(thanktag_queries)):
    thanktag_query = thanktag_queries[i]
    thanktag_query.execute_query()
    if thanktag_query.contains_thanktag():
        thanktag = thanktag_query.soup_result[0]

        if i == len(thanktag_queries)-1:
            if not contains_carriage_return(thanktag.text):
                thanktag = thanktag_query.soup_result[-2]

        break
if thanktag != None and not contains_carriage_return(thanktag.text):
    do_something()

Both pieces of code, which attempt to be logically synonymous, address OP's initial concern with keeping the "soup finds" DRY. That is, the parameters of each soup find should only be hardcoded once instead of twice as in the original code.

The pieces of code accomplish keeping DRY by specifying the find parameters in a list called thanktag_queries. The list is composed of elements that each correspond to hardcoded information of the first four if-statements in the OP's code. This hardcoded information contains three elements (hence the 3-tuples in the case of the Without Helper Class): the target tag, the soup options (in this case the target class), and the threshold that the length of the executed soup query needs to exceed to deem that the thanktag exists in those DOM elements.

So far I've talked about the structure of the outside of the for-loop. What about within the for-loop? The structure within the for-loop differs in two ways from a direct translation of the structure from the OP's if statements. First, find_all(...)[0] is used in place of find(...) because the find_all(...) needs to be executed anyway. Second, the special case from the original fourth if-statement is handled in reverse (i.e. it uses find_all(...)[0] as default instead of find_all(...)[-2]). This choice was made because using find_all(...)[0] matches the pattern of the other if statements. This makes the code more brief, and in this case makes it more understandable.

At the end of the code, I use thanktag == None instead of thanktag in globals() in case of an unintended use of the thanktag variable in that namespace.

I wrote the "With a Helper Class" so that the code is written in a style that is more human readable and self-documenting.

\$\endgroup\$
  • \$\begingroup\$ Another comment: It should be if i == len(thanktag_queries):, because I only have troubles with carriage returns when they are in "li", {"class":"fn"}, i.e. the last case. \$\endgroup\$ – MERose Apr 19 '15 at 23:26
  • \$\begingroup\$ Because of zero based indexing, len(thanktag_queries)-1 refers to the last case. \$\endgroup\$ – davidhwang Apr 20 '15 at 16:16
  • \$\begingroup\$ Well, my script only did what I wanted after I removed -1. \$\endgroup\$ – MERose Apr 20 '15 at 16:43

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.