6
\$\begingroup\$

I just started learning Python and I wrote my first useful script for work. I did a bunch of the basic tutorials and really enjoyed learning Python so far.

I am looking for any advice on how to make things more pythonic. What areas can I improve going forward? I want to make this script better/move on to my new project but I don't want to build off bad fundamentals.

My script works and I am using it. My script goes to a set of websites using the selenium webdriver to pull all the links down to a list. I then delete the duplicates. Then I use the requests module to verify a 200 response code.

I incorporated multiprocessing into it because the first version took way too long, 5+ hours to scan on 7300 links. I got the script time down to about an hour.

CustomFunctions.py

import requests
from selenium import webdriver
import time
import multiprocessing

def get_links(x):
    chrome_options = webdriver.ChromeOptions()
    chrome_options.add_argument('--disable-application-cache')
    driver = webdriver.Chrome('/Desktop/project/SiteCheck/LinkCheckV03/chromedriver', chrome_options=chrome_options)
    driver.get(x)
    links = driver.find_elements_by_xpath('//*[@href]')
    time.sleep(4)
    return links

def check_links(links):
    try:
        r = requests.get(links)
        rc = r.status_code
        strRc = str(rc)
        result = links, strRc
        return result
    except Exception as e:
        logz = open('exception.log', 'w')
        logz.write(str(e) + '\n')

def main(func, mlist):
    pool = multiprocessing.Pool(4)
    results = pool.map(func, mlist)

    pool.close()
    pool.join()

    return results

LinkCheck.Py

from CustomFunctions import get_links, check_links, main
import fileinput
import sys

#redircting stdout to fie
old_stdout = sys.stdout
log_file = open("output.log","w")
sys.stdout = log_file


#Gateways
home = 'http://www.bonton.com'
#brands = 'http://www.bonton.com/sc1/brands/'
women = 'http://www.bonton.com/sc1/women/'
shoes = 'http://www.bonton.com/sc1/shoes/'
handbagsAccessories = 'http://www.bonton.com/sc1/handbags-accessories/'
jewelryWatches = 'http://www.bonton.com/sc1/jewelry-watches/'
beautyFragrance = 'http://www.bonton.com/sc1/beauty-fragrance/'
juniors = 'http://www.bonton.com/sc1/juniors/'
men = 'http://www.bonton.com/sc1/men/'
babyKids = 'http://www.bonton.com/sc1/baby-kids/'
bedBath = 'http://www.bonton.com/sc1/bed-bath/'
homegw = 'http://www.bonton.com/sc1/home/'
furniture = 'http://www.bonton.com/sc1/furniture/'
#clearence = 'http://www.bonton.com/clearence/'

#Master List of Links
masterList =[]

#### Fetch Links from host

#Home GW Populate Links
Homelist = get_links(home)

for ii in Homelist:
    masterList.append(ii.get_attribute('href'))

print('Total number of links on Homepage: ', len(Homelist))

#Women GW Populate List
Womenlist = get_links(women)

for ii in Womenlist:
    masterList.append(ii.get_attribute('href'))

print('Total number of links on Womens GW: ', len(Womenlist))

#Shoes GW Populate List    
Shoeslist = get_links(shoes)

for ii in Shoeslist:
    masterList.append(ii.get_attribute('href'))

print('Total number of links on Shoes GW: ', len(Shoeslist))

#HandbagandAcc. GW Populate List
handbagsAccessorieslist = get_links(handbagsAccessories)

for ii in handbagsAccessorieslist:
    masterList.append(ii.get_attribute('href'))

print('Total number of links on Handbags&Accessories GW: ', len(handbagsAccessorieslist))

#jewelryWatches GW Populate List
jewelryWatcheslist = get_links(jewelryWatches)

for ii in jewelryWatcheslist:
    masterList.append(ii.get_attribute('href')) 

print('Total number of links on Jewelry&Watches GW: ', len(jewelryWatcheslist))

#beautyFragrance GW Populate List
beautyFragrancelist = get_links(beautyFragrance)

for ii in beautyFragrancelist:
    masterList.append(ii.get_attribute('href'))

print('Total number of links on Beauty&Fragrance GW: ', len(beautyFragrancelist))

#juniors GW Populate List
juniorslist = get_links(juniors)

for ii in juniorslist:
    masterList.append(ii.get_attribute('href'))

print('Total number of links on Juniors GW: ', len(juniorslist))

#men GW Populate List
menlist = get_links(men)

for ii in menlist:
    masterList.append(ii.get_attribute('href'))

print('Total number of links on Mens GW: ', len(menlist))

#babyKids GW Populate List
babyKidslist = get_links(babyKids)

for ii in babyKidslist:
    masterList.append(ii.get_attribute('href'))

print('Total number of links on Baby&Kids GW: ', len(babyKidslist))

#bedBath GW Populate List
bedBathlist = get_links(bedBath)

for ii in bedBathlist:
    masterList.append(ii.get_attribute('href'))

print('Total number of links on Bed&Bath GW: ', len(bedBathlist))

#homegw GW Populate List
homegwlist = get_links(homegw)

for ii in homegwlist:
    masterList.append(ii.get_attribute('href'))

print('Total number of links on Home GW: ', len(homegwlist))

#furniture GW Populate List
furniturelist = get_links(furniture)

for ii in furniturelist:
    masterList.append(ii.get_attribute('href'))

# Print link totals and get rid of dupelicates    
print('Total number of links on Furniture GW: ', len(furniturelist))
print('Total number of links before duplicates are removed:', len(masterList))
MasterListNoDupes = list(set(masterList))
print('Total number of links after duplicates are removed: ', len(MasterListNoDupes))

#removes empty elements in MasterListNoDupes
#MasterListNoDupes.remove('None')

#exextue the check_links commands with multiprocesing
if __name__ == '__main__':
    logd = open('linklist.log', 'w')
    line = main(check_links, MasterListNoDupes)
    for items in line:
        logd.write(str(items) + '\n')
    logd.close()

#Only print links that do now have a 200 response code 
with open('linklist.log', 'r') as searchfile:
    for line in searchfile:
        if '200' in line:
            pass            
        else:
            print(line)

#ending stdout logging to file            
sys.stdout = old_stdout

log_file.close()
\$\endgroup\$

migrated from stackoverflow.com Apr 24 '17 at 12:14

This question came from our site for professional and enthusiast programmers.

  • 2
    \$\begingroup\$ IMO using ii instead of i is kinda ugly. \$\endgroup\$ – dot.Py Apr 24 '17 at 12:16
  • \$\begingroup\$ Thank you, I will consider this and update my script. I appreciate you taking the time to help me. \$\endgroup\$ – Travis Bathurst Apr 24 '17 at 21:26
  • \$\begingroup\$ This was my solution using only curl in PHP; The reason I am including it is I show using just curl how to check for valid pages. It is possible the link will give you a temporary redirect but is still valid so you should check this condition. In addition my solution will only capture the header and will not load the full page. This will make your script super fast. I am not sure if you can do it in python but perhaps there is some useful pieces there. In my case I was using it to scan site links of around 300,000 links so speed was a concern. stackoverflow.com/a/21032909/1361532 \$\endgroup\$ – Shawn Apr 24 '17 at 21:31
  • \$\begingroup\$ (cont.) My solution will also if you check the code status on say a permanent redirect, to capture the new updated url and update yours. \$\endgroup\$ – Shawn Apr 24 '17 at 21:39
  • \$\begingroup\$ Thank you, not 100% sure why I started with Python. I had a buddy in high that was a real big python nerd so I figured that I would start learning that first. The reason I wanted to use selenium was because eventually I am going to build off this to check other features of my site and selenium seems to be a real good webkit for website testing. I do want to learn PHP. Figured I would get my feet wet with python first. Thank you for responding. \$\endgroup\$ – Travis Bathurst Apr 24 '17 at 21:40
3
\$\begingroup\$

Your LinkCheck.py can be greatly simplified:

from CustomFunctions import get_links, check_links, main
import fileinput
import sys

#redirecting stdout to file
old_stdout = sys.stdout
log_file = open("output.log","w")
sys.stdout = log_file


#Gateways
gateways = {'Homepage': 'http://www.bonton.com',
            ...,
            'Handbags&Accessories GW': 'http://www.bonton.com/sc1/handbags-accessories/',
            ...}

# Fetch Links from host
all_links = []
for gateway, url in gateways.items():
    links = get_links(url)
    print('Total number of links on {}: {}'.format(gateway, len(links)))
    all_links.extend(link.get_attribute('href') for link in links)

# Print link totals and get rid of duplicates    
print('Total number of links before duplicates are removed:', len(all_links))
all_links = list(set(all_links))
print('Total number of links after duplicates are removed: ', len(all_links))

# execute the check_links commands with multiprocesing
if __name__ == '__main__':
    logd = open('linklist.log', 'w')
    line = main(check_links, MasterListNoDupes)
    for items in line:
        logd.write(str(items) + '\n')
    logd.close()

# Only print links that do now have a 200 response code 
with open('linklist.log', 'r') as searchfile:
    for line in searchfile:
        if '200' in line:
            pass            
        else:
            print(line)

# ending stdout logging to file            
sys.stdout = old_stdout

log_file.close()

That being said, you should have a look at Python's official style-guide, PEP8. Also, it is quite weird to have code (especially including print) outside of the if __name__ == '__main__': guard, so I would move it inside of it (or even inside of a main function`), unless this interferes with the multiprocessing.

\$\endgroup\$
  • 1
    \$\begingroup\$ Than you very much. This makes sense. I originally had my gateway list in a dictionary and had trouble getting something to return from one of my functions correctly. This looks a lot cleaner. I think in the future it'd be best if I figure out the flow of my program before I start and it may be able to plan a little better ahead of time to make it look cleaner and be more efficient. \$\endgroup\$ – Travis Bathurst Apr 24 '17 at 21:31
3
\$\begingroup\$

Adding to the Graipher's excellent answer, here are some selenium specific points:

  • you are not closing the browser instances started by the webdriver
  • using a CSS selector to get all the elements having href attribute should be generally faster:

    driver.find_elements_by_css_selector("[href]")
    
  • I don't think there is a need to wait for 4 seconds after getting the links

  • you can actually return the list of links instead of the list of WebElements from the get_links() function:

    links = [link.get_attribute("href") 
             for link in driver.find_elements_by_css_selector("[href]")]
    
  • getting the links this way actually involves a lot of JSON over HTTP overhead and, if performance matters, check out this discussion on possible faster ways to do this

FYI, check out scrapy and scrapy-splash as an alternative toolset to approach the problem.

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for responding. This is real useful information about selenium. I plan on building off this script to do some other site check test for my company so having a solid foundation with efficient code will come in hand in the future. I am going to rewrite it based on what I learned from all these comments. \$\endgroup\$ – Travis Bathurst Apr 24 '17 at 21:33

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.