I'm just beginning with Python and programming, so been trying to get as much experience reading code as possible.

The script mentioned below do grab images from URLs and put them in into a folder. The code is tested and works fine. Is there a way to shorten/improve this code? Any ideas/suggestions are appreciated.

import urllib2
from os.path import basename
from urlparse import urlsplit
from bs4 import BeautifulSoup # for HTML parsing

global urlList
urlList = []

print url
global urlList
if url in urlList: # prevent using the same URL again
return
urlList.append(url)
try:
except:
return

soup = BeautifulSoup(''.join(urlContent))
imgTags = soup.findAll('img')
for imgTag in imgTags:
imgUrl = imgTag['src']
try:
fileName = basename(urlsplit(imgUrl)[2])
output = open(fileName,'wb')
output.write(imgData)
output.close()
except:
pass

# if there are links on the webpage then recursively repeat
if level > 0:
try:
except:
pass

# main


Python version 2.6.

• You can also simplify the way you look for the links to follow:

for link in soup.select("a[href]"):


Here, we are enforcing the a elements to have href values, not checking for elements to be found (since the loop body would just not be executed in this case).

• As far as managing the urls you've already visited:

• don't use globals
• use a set instead of a list for faster lookups
• It's also a good practice to always specify the parser BeautifulSoup uses under-the-hood:

soup = BeautifulSoup(urlContent, "lxml")
# or soup = BeautifulSoup(urlContent, "html.parser")
# or soup = BeautifulSoup(urlContent, "html5lib")

• follow PEP8 recommendations - specifically, naming is agreed to be in lower_case_with_underscores format, not camelCase

• no need to join the urlContent - simply pass urlContent to BeautifulSoup

### Alternative solution

I would implement it this way:

try:
from urlparse import urljoin
except ImportError:
from urllib.parse import urljoin

import requests
from bs4 import BeautifulSoup

class Scraper:
def __init__(self):
self.visited = set()
self.session = requests.Session()
self.session.headers = {"User-Agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.109 Safari/537.36"}

requests.packages.urllib3.disable_warnings()  # turn off SSL warnings

def visit_url(self, url, level):
print(url)
if url in self.visited:
return

content = self.session.get(url, verify=False).content
soup = BeautifulSoup(content, "lxml")

for img in soup.select("img[src]"):
image_url = img["src"]
if not image_url.startswith(("data:image", "javascript")):

if level > 0:

local_filename = image_url.split('/')[-1].split("?")[0]

r = self.session.get(image_url, stream=True, verify=False)
with open(local_filename, 'wb') as f:
for chunk in r.iter_content(chunk_size=1024):
f.write(chunk)

if __name__ == '__main__':
scraper = Scraper()
scraper.visit_url('http://www.yahoo.com', 1)


Aside from things mentioned above, here are some other applied changes:

• using requests third-party library with a shared session
• Python 2 and 3 compatible
• using class for sharing "session" and a set of visited urls
• if __name__ == "__main__": is used to avoid the code being executed on import
• changed the way image filename is determined (probably still not the best way)
• handling relative urls as well as absolute
• Thanks for you answer, have you tested you code, it gives this error : IOError: [Errno 22] invalid mode ('wb') or filename: 'p?c1=2&c2=7241469&c7=https%3A%2F%2Fwww.yahoo.com%2F&c5=2023538075&cv=2.0&cj=1' – V_V Jun 28 '17 at 20:58
• @V_V I've added the .split("?")[0] to the image filename definition to get rid of http get parameters in the image url and filename - give it a try. Thanks. – alecxe Jun 28 '17 at 21:01
• Thanks again :) . you code seems to be much faster. However it always gives error please for example try yahoo.com or hrp.org.uk. – V_V Jun 28 '17 at 21:19
• What's the point of if chunk:? Is it really necessary to avoid writing nothing to the file? – Aran-Fey Jun 28 '17 at 22:13
• @Rawing yeah, you are right, good finding! Fixed. Thanks. – alecxe Jun 29 '17 at 1:32

# Catching exceptions

It's generally not a good idea to use try-except blocks when you aren't specifying what Exceptions you want to catch, as you've done here:

try:
except:
return


As well as here:

try:
fileName = basename(urlsplit(imgUrl)[2])
output = open(fileName,'wb')
output.write(imgData)
output.close()
except:
pass


You should specify exactly which Exceptions you are catching. If you don't, then every single Exception raised under the try block will be ignored, including ones caused by errors in your code. Ideally, you should also output to the user what exactly went wrong. Here's an example:

try:
some_function(argument1, argument2)
except (ExceptionA, ExceptionB) as exception:
print "X thing failed due to Y. Using Z instead may solve this"


# Opening files

In your code you open files using the standard open and close methods provided, like so:

output = open(fileName,'wb')
output.write(imgData)
output.close()


While there's not much that's inherently wrong with doing this, it's almost always better (and safer) to use a context manager to open files. This means that the above code, using a context manager, would become this:

with open(fileName, 'wb') as output:
output.write(imgData)


# Nitpicks

Other than the two things listed above, I really only have two other criticisms of your code, those two criticisms being purely stylistic in nature:

1. function names should be in snake_case, not camelCase
2. variable names should also be in snake_case and not camelCase

These two naming criticisms are documented in PEP8, the official style guide for Python. I'd recommend taking a look at it.

The use of global is generally discouraged and isn't needed in this example. You can leave out global and the code will function just fine.