89

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: ...


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('...


14

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


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'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

I have no familiarity with any of the libraries used here, so I can't comment on their usage. What I will mention though is the giant chunk of profile.set_preference calls in the middle of the script. It would be much cleaner and less repetitive to save the string/bool pairs of options as a dictionary (or another "paired" structure), then just iterate over ...


8

As for the code below, I'd appreciate any feedback, particularly in regards to threading You might want to use a ThreadPoolExecutor to manage your threads. This approach has two advantages: The executor can be used as a context manager to make sure all the threads are joined. It lets you limit the number of threads in the thread pool. Example: with ...


8

Not a problem in terms of functionality (seems to have been covered pretty well already) but in terms of clarity/readability: Misleading name get_penultimate It appears to functionally be get_last (get_ultimate, if you will), so there is a clear mismatch between this name and the implemented functionality. The functionality makes sense, so the name appears ...


7

You should probably start with a profiler whenever you have unexplained performance problems, that should quickly point you to the functions or areas that take most of the runtime. That said, I immediately noticed the json.load and json.dump are called more than once. That seems to be at least a first candidate for optimisation. Either start keeping ...


7

Just wanted to add that you may want to consider using Polly instead of rolling your own retry mechanism - this is a library built specifically for this kind of retry mechanism (as well as many more complicated scenarios). Using Polly would look something like the following. Note that I have renamed the attempts parameter to retryAttempts to make it clear ...


6

While decorators are fun to learn about (especially when you get to decorators taking arguments and class decorators) and they can be quite useful, I think this decorator should not be one. Sorry. Your code becomes much easier to read and understand by making this into two functions, one that gets the links and one that gets the title from a link, which you ...


6

If you find yourself, doing something over and over again, loops are often the answer. Your code rewritten with a loop: import re import requests from bs4 import BeautifulSoup urls = { "Charms": "https://us.pandora.net/en/charms/?sz=30&start={}&format=page-element", "Bracelets": "https://us.pandora.net/en/bracelets/?sz=30&start={}&...


6

After our initial comment-review-update sequence/loop there is not much left for a review but still, one more thing. You can get rid of the if entirely when you add a when filter to the excpetion: catch (WaitTaskTimeoutException) when (attempts > 0) { //add some delay await Task.Delay(10000); return await ...


5

This function: def get_csv_header(top_numb): fieldnames = ['Hashtag','Active Days Ago','Post Count','AVG. Likes','MAX. Likes','MIN. Likes','AVG. Comments','Hashtag URL','Post Ready Tag'] return fieldnames has a few issues. top_numb is unused, so delete it. You can both construct and return the list in the same statement, but due to its length I ...


5

I would separate the "input" (finding titles and descriptions) from the output (writing to screen or file). One good way to do that is to use a generator: from bs4 import BeautifulSoup import requests import csv def soup(): source = requests.get("https://www.youtube.com/feed/trending").text soup = BeautifulSoup(source, 'lxml') def find_videos(soup)...


5

Python has an official style-guide, PEP8. It recommends using lower_case for variables, functions and methods. I would encourage you to use requests.get instead of urllib.request. It can take care of urlencoding the parameters for you. You can make pull_from_arxiv a generator to save a few lines. BeautifulSoup can be sped up using the lxml parser. on_get can ...


5

Why would you use stackexchange.com and only take ones with the title of stackoverflow.com? That's a huge waste. You could change your url from https://stackexchange.com/questions?tab=realtime to https://stackoverflow.com/questions?tab=realtime. However, actually visiting the website the way you are is hacky and overkill. Instead of web scraping, use the ...


5

Python uses snake_case, not camelCase for variable and function names. csvFormat for example, should be csv_format. In two places you do this weird thing: with open(. . .) as file: . . . file.close with already closes file, so there's no need to manually close it. file.close doesn't do anything, and a good IDE would warn you of that. You need to add ()...


4

I have made some changes to make it neater and more efficient. See the end of this response for the final solution. But I'll start with a few comments. There is no point making your class inherit the object class, especially if you're working with Python 3. class Page(object): The soup() method seems to be called indiscriminately causing BeautifulSoup to ...


4

Just going to focus on your main program, not cleanname.py. Import Order: This one is a personal preference. I like to have my imports ordered alphabetically. You can decide to follow this practice or not. Docstrings: Docstrings are used to help identify what your method is supposed to do. You should include one in every method you write. Unused variables/...


4

Whitespace formatting Apply a linter that will give you PEP8 suggestions. Among other things, it will suggest the following: import os,gc,queue There should be spaces after those commas. It'll also suggest that there be one line per import. class add_line: should be class AddLine: and have a couple of newlines before it. else : shouldn't have a ...


4

Ignoring threading, it's usually best to benchmark your code on a single thread first, then move to multiple threads as necessary. Some glaring things that could yield slower performance: You are not using a Session for requests. There is not a guarantee that sessions are thread-safe, but you could allocate a session for each thread, giving each thread it's ...


3

not exhaustive but some feedback. There are some big "No No"s in this code as well as some minor formatting things. First the big "No No"s. 1) Do not use list comprehension unless you plan do use the result: [links_to_map.append(i) for i in Page(base_url,url).links()] Do this in a normal for loop. 2) DO NOT change a list you are iterating over (i.e. ...


3

IMO the answer highly depends on how you are planning to extend the functionality of your class. If its only function is fetching the username, it's probably better to transform it into a function. Class is an overkill. A few thoughts if you're going to expand it: You can take a look at the Stack Exchange API and see if it matches your needs if you haven't ...


3

Use f-strings This: "Salt Bot is now watching {} matches.\n".format(matches) can be f'Salt Bot is now watching {matches} matches\n' and so on for your other format calls. Use more subroutines watch() is quite long. You should break it up into multiple functions. Redundant else This: if len(current_bet_status) == 0: ...


3

Some quick suggestions: The requests module can urlencode strings for you if you use the params keyword: import requests cities = ["Menlo Park, CA"] pages = range(1, 3) url = "https://www.facebook.com/careers/jobs/" for city in cities: for page in pages: params = {"page": page, "results_per_page": 100, "locations[0]": city} response =...


3

You cannot use a return in a __construct() method. If you need to check for any "badness", you can use throw/try/catch.Relevant resources: Return false from __constructor & PHP - constructor function doesn't return false (In the end, I support Sam's advice about writing class constants.) Regarding your regex patterns, they can be simplified. /\r\n|\r|\...


3

General Observations instead of manually looking for a tag a desired position and handling pos increment in the loop, I think you could just simply get the value by index: curpos = tags[int(pos_1) - 1].get('href', None) count = count - 1 could be simplified as count -= 1 follow the PEP8 lower_case_with_underscores variable naming guideline what if you ...


Only top voted, non community-wiki answers of a minimum length are eligible