Я создал простой веб-скрапер, который извлекает превью новостных статей из природа.com и сохраняет каждую статью в файл, содержащий текст предварительного просмотра статьи.
Я учусь самостоятельно, поэтому я хотел бы получить отзывы о структуре кода, чистоте кода и дизайне ООП (или даже о том, является ли ООП лучшим подходом здесь, поскольку я впервые думаю об использовании ООП в проектах).
Пожалуйста, может кто-нибудь просмотреть мой код и предложить области для улучшения?
import re
import string
import requests
from bs4 import BeautifulSoup
class Article:
def __init__(self, title, body):
self.__title = title
self.__format_title()
self.__body = body.strip()
@property
def title(self):
return self.__title
@property
def body(self):
return self.__body
def __format_title(self):
return re.sub(f'[{string.punctuation}]', '', self.__title).strip().replace(' ', '_')
class ArticleScraper:
def __init__(self, url):
self.url = url
self.response = None
self.__soup = None
self.__articles = []
@property
def articles(self):
return self.__articles
def save(self):
self.__scrape()
self.__get_articles_from_soup()
self.__write_articles_to_file()
def __scrape(self):
self.__make_request()
self.__soup = BeautifulSoup(self.response.content, 'html.parser')
def __get_articles_from_soup(self):
articles = self.__soup.find_all('article')
for article in articles:
article_type = article.find('span', {'data-test': 'article.type'}).text.strip()
if article_type == 'News':
link = article.find('a', {'data-track-action': 'view article'}).get('href')
if link:
article_url = f'https://www.nature.com{link}'
resp = requests.get(article_url)
resp.raise_for_status()
article_soup = BeautifulSoup(resp.content, 'html.parser')
article_title = article_soup.find('title').text
# cannot get article body as nature.com now behind paywall, get teaser text instead
article_body = article_soup.find('div', {'class': 'c-article-teaser-text'}).text
self.__articles.append(Article(article_title, article_body))
def __write_articles_to_file(self):
for article in self.__articles:
with open(f'{article.title}.txt', 'w', encoding='utf-8') as f:
f.write(article.body)
def __make_request(self):
self.response = requests.get(self.url, headers={'Accept-Language': 'en-US,en;q=0.5'})
self.response.raise_for_status()
if __name__ == '__main__':
url_to_request = "https://www.nature.com/nature/articles?sort=PubDate&year=2020&page=3"
webpage_scraper = ArticleScraper(url_to_request)
try:
webpage_scraper.save()
print('Saved articles:')
[print(f'{article.title}.txt') for article in webpage_scraper.articles]
except requests.HTTPError:
print(f'The URL returned {webpage_scraper.response.status_code}')
razzleDazzle
1 ответ
def __init__(self, title, body):
self.__title = title
self.__format_title()
...
def __format_title(self):
return re.sub(f'[{string.punctuation}]', '', self.__title).strip().replace(' ', '_')
Это не имеет смысла. В этом представлении нет модульных тестов, что, вероятно, соответствует отсутствию модульных тестов в исходном коде. Простой тест должен был сразу обнаружить пропущенное задание.
Помощник по формату заголовка хорош. (Ну, хорошо, его имя должно начинаться только с одного _ подчеркивание, что угодно.) Но тогда вызывающая сторона не может присвоить очень хорошее возвращаемое значение. Вы отформатировали заголовок, а потом выбросили результат.
В более общем смысле, здесь есть множество других идентификаторов, которые начинаются с двойного подчеркивания вместо одинарного, как описано в
ПЭП-8.
Непонятно, почему у нас title / body добытчики. Пожалуйста, поймите, что Python — это не Java. Были все взрослые здесь. Используйте ноль или один знак подчеркивания в начале, чтобы описать, является ли атрибут общедоступным или закрытым.
Спасибо за выбор существительного, ArticleScraperчтобы назвать класс.
И, конечно же, мы обычно предпочитаем глаголы для имен методов.
в ArticleScraper конструктор, спасибо за введение четырех атрибутов; это полезный справочник, который помогает читателю понять остальную часть класса.
Опять же, articles getter кажется бессмысленным. Это не похоже на то, что он возвращает .copy() или что-нибудь. Удали это.
У вас была хорошая интуиция (особенно когда вы использовали идиому повышения статуса), но детали не сработали.
def __scrape(self):
self.__make_request()
self.__soup = BeautifulSoup(self.response.content, 'html.parser')
def __make_request(self):
self.response = requests.get(self.url, headers={'Accept-Language': 'en-US,en;q=0.5'})
self.response.raise_for_status()
Предпочтительнее формулировать помощника следующим образом:
@staticmethod
def _make_request(url):
response = requests.get(url, ...)
response.raise_for_status()
return response
И затем вызывающий назначает
self.response = _make_request(self.url)
Это имеет несколько преимуществ. Связи меньше, хелпер проще понять, и мы можем легче писать для него модульные тесты.
Изображение в формате JPEG днастройка качества, ;q=0.5здесь не имеет смысла.
(Извините, моя ошибка, пожалуйста, смотрите исправление в комментариях).
[print(f'{article.title}.txt') for article in webpage_scraper.articles]
Здесь вы оцениваете понимание списка побочных эффектов и отбрасываете список. Что не является ясным способом выразить намерение автора. Пожалуйста, не делай этого. Следуйте обычной идиоме:
for article in webpage_scraper.articles:
print(...)
Общий?
Эта заявка не содержала модульных тестов, и в ее текущем состоянии, по-видимому, она не подходит для написания простых тестовых методов, которые используют существующие помощники. Нулевые тесты не повышают нашу уверенность в правильности этой кодовой базы.
Для небольшого количества кода я должен сказать, что есть много из
связь
происходит здесь. Совсем уж слишком. Я нашел это слишком сложным для чтения save(), так как он не использует формальных параметров, передавая вещи через атрибуты объекта, которые в этом контексте по сути являются глобальными переменными. Все оценивается по побочным эффектам. Мне казалось, что следующая строчка кода, которую я прочитаю, должна быть написана на Фортране. COMMON заявление.
Вы сохраняете функции небольшими; это хорошо. Вы пытаетесь придерживаться того, чтобы заставить их делать
одна вещь, также хорошо. Следующее, что вы должны попробовать попрактиковать, это понятие передачи одного или двух соответствующих параметров и получения обратно результата, который вы назначаете (или передаете следующему помощнику). Это улучшит ясность вашего кода, ваших намерений.
В его нынешнем состоянии я бы не хотел делегировать или принимать задачи обслуживания кода для этой кодовой базы.