88
Stop hammering XKCD server
You basically wrote a denial-of-service attack, since you try to connect as fast and as often as possible to the server.
By sharing this code on CodeReview, it becomes a distributed denial-of-service.
Just in case it wasn't clear : don't do that.
I suppose the load of XKCD servers is high enough that your code won't make any ...
40
Why web-scrape, when you can get the data properly through the YouTube Data API, requesting the mostpopular list of videos? If you make a GET request to https://www.googleapis.com/youtube/v3/videos?key=…&part=snippet&chart=mostpopular, you will get the same information in a documented JSON format.
Using the Python client, the code looks like:
...
28
That's a beautiful excuse for writing code, and the final product is quite nice as well.
★ First of all, congrats to your Java-ridden mind for not forcing classes into Python where they aren't needed.
★ You import but do not use OrderedDict and operator. Remove unused imports.
★ It is customary to write your code in a way that allows it to be used as ...
26
import*:
Please avoid importing all modules at all costs. Import each module you use separately
Regex ಠ_ಠ
Regex is evil, the worst. seriously. stop it. right now. kill python.exe right now and change it.
You use beautifulsoup, a dom parsing library, literally a few lines later, but you choose to use regex.
You sir, are evil.
I would suggest looking ...
23
Cargo Fmt
There's a very common tool accessible through cargo which can format all of the code in your project according to Rust's official style guide. Many major open source Rust libraries use this tool (and even enforce it through CI on pull requests), which you can access through cargo fmt. You can also customize its output using a .rustfmt config file. ...
22
I see a few, actually, a number of problems in this code. I am afraid this will be something of a scathing review, in part because you mention this is intended to be for a tutorial....
I see a number of call them 'critical' issues. Then also a number of lesser issues. The critical ones first:
Use Case
Why not use an Iterator? Since an Iterator would do ...
22
You do not need HTML parsing at all. OEIS has a nice JSON output format.
https://oeis.org/search?fmt=json&q=id:A000045
So the core functionality of your program can be written as something like
import sys
import urllib2
import json
f = urllib2.urlopen("https://oeis.org/search?fmt=json&q=id:%s" % sys.argv[1])
doc = json.loads(f.read())
comment = ...
20
After I've gone through your code, I decided to rewrite it because using regex to parse html isn't a good idea at all.
When you're parsing html, is recommended to use BeautifulSoup, so I'll rewrite your code using it.
First, I'll start from the imports:
from bs4 import BeautifulSoup as bs
import requests
Nothing too fancy so far, we're importing the ...
18
import urllib
import re
import os
# The parameter is the url the crawler is gonna crawl.
That's a pretty useless comment. The parameter is named url. So I already know it is the URL.
def crawler(url):
Seeing as this is a function, it should really be named as a verb. The function is an action, not a thing.
# The crawler find links by utilizing ...
16
import json
import re
import urllib2
from scraper_tools import soupify, getHtml, buildJsonPostRequest
__author__ = 'ecnalyr'
def getImageLinkFromDiv(div):
The python style guide recommends lowercase_with_underscores for function names
"""Expects a BeautifulSoup div from {website's} new product page"""
try:
fullSrc = div.find('img')['src']
...
16
Assignment Branch Condition size for fetch_images is too high. [22.41/15]
What that warning is saying is that, according to rubocop's standard, you have too many assignments, branches, and conditionals (ABCs) in that method. ABCs are associated with code that is complex and harder to reason about. It can also indicate that a method is trying to do too much, ...
15
Context manager
You open a file at the beginning of the program and close it explicitly at the end.
Python provides a nice way to allocate and release resources (such as files) easily: they are called Context managers. They give you the guarantee that the cleanup is performed at the end even in case of exception.
In your case, you could write:
with open('...
15
Globals
As it is, there would be an advantage to LOGO being a local instead of a global - it's only used by show_logo, and moving it there would clean up the global namespace.
That said (and as others have pointed out), it's fairly common to see stuff like this at the top of Python files in global scope. However, the larger issue is that if you move it to ...
13
There's a lot to improve about your code, and I'll start from the top. I'll also rewrite some of the logic you had there because IMO it's a bit clumsy and hard to understand.
Imports
Don't use urllib and requests at the same time. Use either one or another. They serve the same purpose.
from bs4 import BeautifulSoup
import os
import random
import requests
...
13
I tried using __ once and found it to produce more problems than it's worth. Take:
def __foo():
return 'foo'
class Bar:
def __init__(self):
self.__baz = __foo()
Bar()
NameError: name '_Bar__foo' is not defined
I'd suggest you follow PEP 8 and only use it for classes that will be subclassed and where you need to prevent name collisions. ...
12
This seems like a cool project, and a nice first introduction to web scraping! I'll cover some general advice first, and then address your main concern: speed.
General
You use two spaces for indentation. PEP-8 recommends 4 spaces (most code editors have the option to automatically convert tabs to spaces, which I highly recommend).
While on the subject, PEP-...
11
Unused variables
x, proc_c, start and JustDOit are set but never used, thus they are unneeded. For instance, write:
def main():
while True:
try:
Scanner(cookie_value)
except:
print "= Sleeping for 1sec and trying again"
time.sleep(1)
by the way, where does cookie_value come from? It is used ...
answered Oct 26 '15 at 13:01
301_Moved_Permanently
27.2k3 gold badges37 silver badges92 bronze badges
11
If you're after speed, I'd suggest scrapy. I was looking for an excuse to try it out and saw your question. When I ran your code on the first 10 pages of the NA leaderboard, it took a little over 4 seconds. Running the below takes about 0.3 seconds, presumably due to initiating all the HTTP requests in parallel:
test.py:
class LolSpider(scrapy.Spider):
...
11
A couple of suggestions:
Use requests instead of urllib. This is mostly a matter of preference but I think you'll find it easier to work with.
An example on how this would look is:
import requests
page = requests.get('http://rabota.ua/zapros/python/%D0%BA%D0%B8%D0%B5%D0%B2').content
Fix the bug
In some cases you'll get:
Traceback (most recent call ...
11
The right tool
As you've said, you are not using the right tool for this task : you can't parse HTML with regexps.
A better approach would be to use an already existing parser like BeautifulSoup.
A simpler container
At the moment, you are putting data in multiple lists to zip them all at the very end. It can be a very nice technique but in our case, you ...
11
First of all, usual things (I feel like I'm suggesting these things in most of the web-scraping related discussions):
initialize session as requests.Session() and use session.get() instead of requests.get() - this would speed things up because the underlying TCP connection will be re-used for subsequent queries to the same domain
use SoupStrainer to limit ...
11
for page in reversed(range(latest_comic - pages + 1, latest_comic + 1)):
print(f"Fetching page {page} out of {latest_comic}")
try:
url = get_images_from_page(f"{BASE_URL}{page}/")
thread = threading.Thread(target=save_image, args=(url, ))
thread.start()
except (ValueError, AttributeError, requests.exceptions.MissingSchema):...
11
As mentioned in a comment by Thomas, another option is to use XKCD's JSON interface rather than scraping HTML:
import requests, time, tempfile, os.path
from shutil import copyfileobj
path = tempfile.mkdtemp()
print(path)
f_name = "{num:04d}-{safe_title}.{ext}"
current_comic = requests.get("https://xkcd.com/info.0.json").json()
# Iterates over numbers from ...
10
I'll start off by saying this is actually really good.
There are a few minor PEP8 errors:
In after_login you call Request. the argument url shouldn't have a space on the right side of the equals.
Another thing in Request. The argument callback needs at least one more indent.
However you should either aligned with opening delimiter or use hanging indents.
...
10
Very nice code in general! Here are a few points, though:
Think about using os.makedirs instead of os.mkdir. The former allows creating nested directories (so os.makedirs("~/long/new/path") will create all other folders in betweetn as well, if needed).
You should never have a bare except. This prevents you to e.g. CTRL-C to abort the program, since it will ...
10
Do not use capitalization for function or variable names. This violates idiomatic python naming conventions (PEP8 link). Try out pylint to catch these kinds of issues
Prefer str.format() over concatenation. str.format() is not only a lot more flexible, but easier to use than concatenation and more efficient for combining more than 2 strings. With str.format()...
10
Welcome to Code Review SE, nice first question! Here are some of my comments, not pertaining to performance:
There's no reason to import modules in the body of scrape_coin_marketcap(). Just do it at the top of the source code file. If you need to be sure the modules are present at all, you can catch ImportError:
import sys
try:
from bs4 import ...
10
I'd definitely look into using an API directly as @200_success suggested to avoid any web-scraping or HTML parsing, but here are some additional suggestions to improve your current code focused mostly around HTML parsing:
you could get some speed and memory improvements if you would use a SoupStrainer to allow BeautifulSoup parse out only the desired ...
9
@abuzittingillifirca made a good point in that you can leave the parentheses off of the -> forms that are just a single element.
html/html-resource is actually quite flexible; it can take a variety of different kinds of arguments, including strings, URLs, input streams, etc. Assuming the url argument to your function is a string, you can take out the ...
9
In general, your code is very well written. PEP8 compliance is very good, names make sense, you have docstrings, you use if __name__ == '__main__:.
Here's a few comments on what I would do differently:
# Get table
try:
table = soup.find('table')
except AttributeError as e:
print 'No tables found, exiting'
return 1
# Get rows
try:
rows = ...
Only top voted, non community-wiki answers of a minimum length are eligible
