8
\$\begingroup\$

I am looking to improve my code structure. In this code it is opening all the links with ?pid= in the url. However, the structure of the code can be improved.

for a in self.soup.find_all(href=True):            
    href = a['href']
    if not href or len(href) <= 1:
        continue
    if href[0] == '/':
        href = (domain_link + href).strip()   
        if href.lower().find("?pid=") != -1:
            href = href.strip()
        elif 'javascript:' in href.lower():
            continue
        elif 'reviews' in href.lower():
            continue
    elif href[:4] == 'http':
        if href.lower().find("?pid=") != -1:
            href = href.strip()
    elif href[0] != '/' and href[:4] != 'http':
        href = ( domain_link + '/' + href ).strip()
        if href.lower().find("?pid=") != -1:
            href = href.strip()
    if '#' in href:
        indx = href.index('#')
        href = href[:indx].strip()
    if href in links:
        continue
    links.append(self.re_encode(href))
self.dict['a_href'] = links
\$\endgroup\$
4
\$\begingroup\$

High-level notes:

  • There are no comments in this code. It would be easier if you provided some explanation for the logic – for example, why do we skip URLs that contain “javascript:” or “reviews” – because it will make the code easier to review and update later.
  • There’s quite a bit of repetition. Pulling that out will make it easier to read and follow.
  • This is a moderately complicated and self-contained block of code. It might be worth pulling it out into its own method/function normalise_url that can be tested independently. If you don’t want to use the URL, return None. Then you have:

    href = normalise_url(a['href'])
    if (href is not None) and (href not in links):
        links.append(href)
    

    And the logic of deciding whether to use the URL, and how much to use, can be managed elsewhere.

Line-specific comments:

  • Your if branches go:

    if href[0] == '/':
        ...
    elif href[:4] == 'http':
        ...
    elif href[0] != '/' and href[:4] != 'http':
        ...
    

    If we reach the second elif, then we know that both of those conditions are true, so we can reduce it to else:.

    Note also that a better way to check if a string starts with a particular substring is to use .startswith().

  • You’re using str.find() to check whether the URL contains the string ?pid=. The better approach is to use in (and indeed, this is recommended by the docs):

    if '?pid=' in href.lower():
        ...
    

    However, it should be noted that on both occasions when you use this, the only thing you do is apply .strip() to a string which has already had .strip()applied. With no arguments, this just removes whitespace – which means the second application does nothing. You could cut out this branch entirely.

  • When you construct the href with the domain link, you could call .lower() there to save you remembering to call it later; i.e.,

    href = (domain_link + href).strip().lower()
    ...
    href = (domain_link + '/' + href).strip().lower()
    
  • When you check whether the URL contains javascript: or reviews, you could compress the condition onto a single line without sacrificing readability:

    if ('javascript:' in href) or ('reviews' in href):
        continue
    
  • When you’re looking for the hash (#) and splitting the string, you’re doing it in quite an unPythonic way. Rather than looking up the index of # in the URL and taking a slice, just take the first part of .split(); i.e.

    href = href.split('#')[0].strip()
    

    This works whether or not there's a hash in the string.

  • I would tidy up the final if condition to check if (href not in links), as it makes the code a little cleaner. It saves you a continue when you’re very close to the end of the loop, and makes it explicit when you’re going to add a URL.

    (Alternatively, you could make links a set, and then you could add every URL and trust Python to sift out the duplicates. Of course, that depends on whether order is important.)

With those comments in mind, here’s my final version of your code:

for a in self.soup.find_all(href=True):            
    href = a['href']
    if not href or len(href) <= 1:
        continue

    if href[0] == '/':
        href = (domain_link + href).strip().lower() 
        if ('javascript:' in href) or ('reviews' in href):
            continue
    elif not href.startswith('http'):
        href = (domain_link + '/' + href).strip().lower()

    href = href.split('#')[0].strip()

    if (href not in links):
        links.append(self.re_encode(href))

I’ve also added some blank lines to break it up into logical units – I think this makes it easier to skim the structure.

\$\endgroup\$
  • \$\begingroup\$ I am getting an error when I tried you recommendation: it says 'missing URL Scheme' do you have any suggestion how to correct this \$\endgroup\$ – Dhrubo Naskar Sep 17 '15 at 16:57
2
\$\begingroup\$

The other answer covers a ton of useful stuff, but I also think whitespace will improve this immeasurably. A block of code with no gaps is much harder to read and communicates nothing compared to separating out different blocks of your logic. Just changing whitespace and nothing else, I'd rewrite it like this:

for a in self.soup.find_all(href=True):            
    href = a['href']
    if not href or len(href) <= 1:
        continue

    if href[0] == '/':
        href = (domain_link + href).strip()   
        if href.lower().find("?pid=") != -1:
            href = href.strip()
        elif 'javascript:' in href.lower():
            continue
        elif 'reviews' in href.lower():
            continue

    elif href[:4] == 'http':
        if href.lower().find("?pid=") != -1:
            href = href.strip()

    elif href[0] != '/' and href[:4] != 'http':
        href = ( domain_link + '/' + href ).strip()
        if href.lower().find("?pid=") != -1:
            href = href.strip()

    if '#' in href:
        indx = href.index('#')
        href = href[:indx].strip()

    if href in links:
        continue
    links.append(self.re_encode(href))

self.dict['a_href'] = links

This separates the flow clearer, tells a reader where the branches are and how they're separated.

\$\endgroup\$

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.