5
\$\begingroup\$

I wrote this program, which purpose is to visit the 18th link on the list of links and then on the new page visit the 18th link again.

This program works as intended, but it's a little repetitive and inelegant.

I was wondering if you have any ideas on how to make it simpler, without using any functions. If I wanted to repeat the process 10 or 100 times, this would become very long.

import urllib
from BeautifulSoup import *

url = raw_input('Enter - ')
if len(url) < 1 :
    url='http://python-data.dr-chuck.net/known_by_Oluwanifemi.html'
html = urllib.urlopen(url).read()
soup = BeautifulSoup(html)

# Retrieve all of the anchor tags
tags = soup('a')
urllist = list()
count = 0
loopcount = 0
for tag in tags:
    count = count + 1
    tg = tag.get('href', None)
    if count == 18:
        print count, tg
        urllist.append(tg)


url2 = (urllist[0])
html2 = urllib.urlopen(url2).read()
soup2 = BeautifulSoup(html2)

tags2 = soup2('a')
count2 = 0
for tag2 in tags2:
    count2 = count2 + 1
    tg2 = tag2.get('href', None)
    if count2 == 18:
        print count2, tg2
        urllist.append(tg2)
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Why do you want to do this "without using any functions"? Where does this restriction come from? \$\endgroup\$ – 301_Moved_Permanently Jun 18 '16 at 7:19
6
\$\begingroup\$

If you are only interested in getting the 18th url from the initial one and then the 18th again there is no reason to go through all of them and count iterations and so on. You can simply access it directly using the indexes. On this computer i do not have BeautifulSoup installed but try this:

import urllib
from BeautifulSoup import *

url_1 = input('') or 'http://python-data.dr-chuck.net/known_by_Oluwanifemi.html'

html_1 = urllib.urlopen(url_1).read()
soup_1 = BeautifulSoup(html_1)

tags = soup('a')
url_retr1 = tags[17].get('href', None)

html_2 = urllib.urlopen(url_retr1).read()
soup_2 = BeautifulSoup(html_2)

tags_2 = soup_2('a')
url_retr1 = tags_2[17].get('href', None)

Should be as simple as that.

\$\endgroup\$
2
  • \$\begingroup\$ Welcome to CodeReview.SE! This is a nice answer making the point I wanted to make later on. \$\endgroup\$ – SylvainD Jun 17 '16 at 16:09
  • \$\begingroup\$ Yours is much more elaborate thus helping the OP more. I tried attacking the problem as i would have if it was originally mine (with the exception of respecting the "rather strange" no functions rule). If answers like yours are preferable, which i guess they are, i shall adjust my aways. Happy to be here! \$\endgroup\$ – Ma0 Jun 17 '16 at 18:50
2
\$\begingroup\$

Style

Python has a style guide called PEP 8 which is definitly worth reading. Your code does not quite respect it on a few aspects (spacing, parenthesis, etc). If you are interested, you'll find various tools to check your code compliancy to PEP 8.


Simplification

len(url) < 1 probably means len(url) == 0 because the length is integer non-strictly positive.

Also, PEP 8 suggests:

For sequences, (strings, lists, tuples), use the fact that empty sequences are false.

so you could probably write : not url


Simplification again

Your loopcount variable doesn't seem to be used.


Organisation

You could write helper method to help you get BeautifulSoup objects from urls.

def get_soup_at_url(url):
    html = urllib.urlopen(url).read()
    soup = BeautifulSoup(html)
    return soup

it will remove the duplicated logic and make your life easier if you want to change something in that part of the logic.


Count like a native

Title borrowed from Ned Batchelder's excellent talk called "Loop Like A Native" which deals with the point I am about to make.

You don't need to keep track of count as you iterate over the tags. You could use the enumerate builtin function. to write :

tags = soup('a')
urllist = list()
for i, tag in enumerate(tags):
    tg = tag.get('href', None)
    if i + 1 == 18:
        print i + 1, tg
        urllist.append(tg)

Also, enumerate takes a start argument so that you can start counting from 1 if you want. Because I do not really understand the logic, it is hard for me to tell what is preferable here. I'll take the solution counting from 1 as it is closer to your current code.

tags = soup('a')
urllist = list()
for count, tag in enumerate(tags, start=1):
    tg = tag.get('href', None)
    if count == 18:
        print count, tg
        urllist.append(tg)

Be lazy

At the moment, you get the href elements of each tag only to see ignore it in most cases. You could move that logic behind your if.

if count == 18:
    tg = tag.get('href', None)

Also, once you've reached the 18th elements, there won't be more of them so you can stop looping.


**Do not import start **

From PEP 8 again :

Wildcard imports ( from import * ) should be avoided, as they make it unclear which names are present in the namespace, confusing both readers and many automated tools.

Because of points above, this is easy to change because we have references to BeautifulSoup in a single place.


To be continued (for instance because you do not need loops at all) but in the meantime, the code looks like :

import urllib
import BeautifulSoup

url = raw_input('Enter - ')
if not url:
    url = 'http://python-data.dr-chuck.net/known_by_Oluwanifemi.html'

def get_soup_at_url(url):
    html = urllib.urlopen(url).read()
    soup = BeautifulSoup.BeautifulSoup(html)
    return soup

soup = get_soup_at_url(url)

# Retrieve all of the anchor tags
tags = soup('a')
urllist = list()
for count, tag in enumerate(tags, start=1):
    if count == 18:
        tg = tag.get('href', None)
        print count, tg
        urllist.append(tg)
        break


soup = get_soup_at_url(urllist[0])
tags = soup('a')
for count, tag in enumerate(tags, start=1):
    if count == 18:
        tg = tag.get('href', None)
        print count, tg
        urllist.append(tg)
        break

To be continued.

\$\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.