Code Review – Indeed Scraper

Jarosław Piszczała
18 Jun, 2020
18 Jun, 2020
- Code review

Jak wiesz, pisanie kodu to nie wszystko. Codzienna praca programisty to także przeglądanie kodu, który zespół chce dostarczyć do wspólnej bazy, a przede wszystkim kodu, który już został dostarczony. Umiejętność odpowiedniego zrozumienia kodu, z którym nie mieliśmy do czynienia może się przydać na rozmowie kwalifikacyjnej, ale przede wszystkim gdy trafimy do projektu, który ma już swoją historię. By lepiej zrozumieć ten proces, przyjrzyjmy się pewnemu repozytorium na Githubie i zróbmy mu code review.

Indeed Scraper

Będziemy przeglądać kod aplikacji Indeed Scraper, która służy do pobierania wpisów z serwisu indeed.com. Skupimy się na zmianach aż do rewizji df8f839, czyli ostatniej rewizji na moment pisania tego artykułu. Postaram się kod rozpatrywać elementami, gdyż branie pod uwagę całego kodu w tym przypadku wygenerowałoby dużo zamieszania w artykule.

Architektura repozytorium

Należą się brawa za plik README.md z opisem, czym jest projekt, oraz jak go używać. Przy takich małych projektach to niestety rzadkość. Testy powinny być w głównym katalogu, w katalogu scraper powinien znajdować się wyłącznie kod tej aplikacji. Również plik main.py powinien tam wylądować, w zamian powinniśmy korzystać z setup.py oraz zdefiniować tam entry_points by łatwo odpalać aplikacje z poziomu terminala. W tym momencie uruchomienie go wymaga zaimportowania modułu i uruchomienia komendy.

Inicjalizacja obiektu klasy

Metoda __init__ służy do inicjalizowania obiektu danej klasy. Jest to miejsce na przyjęcie argumentów oraz zdefiniowanie innych, które będą potrzebne przez naszą logikę. Zdecydowanie nie jest to miejsce na wywoływanie metod klasy. Aktualna implementacja powoduje, że po stworzeniu obiektu klasy Scraper dzieje się magia, i wszystko pod skorupką się wykonuje. Wywołanie konkretnych metod tej klasy powinno być jawne.

1
    class Scraper:
2
        def __init__(self, job_name: str, location: str, radius: int):
3
            self.job_name = job_name
4
            self.location = location
5
            self.radius = radius
6
            self._url = ''
7

8
            self.page = None
9
            self.page_number = 0
10
            self.get_content()
11

12
            self.is_skipped = ''
13
            self.offers = {}
14
            self.number_of_offers = 0
15

16
            self.soup = BeautifulSoup(self.page.content, 'html.parser')
17
            self.result = None
18

19
            self.__find_job_offers()

Operacje na zmiennych obiektu

Mamy tu przykład nadmiarowych metod. Jeżeli programista chciał ograniczyć dostęp bezpośredni użytkownika do zmiennej _url, lepszym rozwiązaniem jest użycie property. Jednak w tym przypadku w metodzie connect przypisujemy poprzez metodę url pod zmienną _url tylko po to, aby następnie poprzez inną metodę go odczytać.

1
        def get_url(self) -> str:
2
            return self._url
3

4
        def set_url(self, url: str) -> None:
5
            self._url = url
6

7
        def connect(self, url: str) -> object:
8
            try:
9
                self.set_url(url)
10
                connection = requests.get(self.get_url())
11
                return connection
12
            except requests.exceptions.RequestException as e:
13
                return e

W tym przypadku, jeżeli potrzebne nam jest zachowanie adresu w obiekcie, wystarczyłoby bezpośrednie przypisanie wartości, jeżeli jest to nam potrzebne, a następnie użycie zmiennej lokalnej spod argumentu url.

1
        def connect(self, url: str) -> object:
2
            try:
3
                self._url = url
4
                connection = requests.get(url)
5
                return connection
6
            except requests.exceptions.RequestException as e:
7
                return e

Obsługa wyjątków

Kolejna uwaga to obsługa wyjątków. O ile powinniśmy pogratulować programiście obsługi konkretnego wyjątku, to sposób jego obsługi nie ma największego sensu. W tym przypadku nasza funkcja zwraca obiekt requests.models.Response, a gdy wystąpi błąd, zwróci ona requests.exceptions.RequestException. Te klasy nie mają tego samego interfejsu, co spowoduje, że nasz kod może generować masę błędów.

1
        def connect(self, url: str) -> object:
2
            try:
3
                self.set_url(url)
4
                connection = requests.get(self.get_url())
5
                return connection
6
            except requests.exceptions.RequestException as e:
7
                return e

Dużo lepsze będzie tutaj zalogowanie błędu poprzez logger i obsługę tej sytuacji w klasie get_content.

1
        def get_content(self) -> None:
2
            self.page = self.connect(f'https://pl.indeed.com/jobs?q=\
3
    {self.job_name}&l={self.location}&sort=date&radius={self.radius}\
4
    &start={self.page_number}')
5
            if not self.page:
6
                return
7
            self.soup = BeautifulSoup(self.page.content, 'html.parser')
8
            self.result = self.soup.find(id='resultsCol')

Formatowanie stringów

Skoro już przy metodzie get_content jesteśmy, to warto zwrócić dodatkowo uwagę na str tworzony jako argument metody connect. Jest on nieczytelny, a przecież w Pythonie tak łatwo to rozbić.

1
        def get_content(self) -> None:
2
            self.page = self.connect(f'https://pl.indeed.com/jobs?q=\
3
    {self.job_name}&l={self.location}&sort=date&radius={self.radius}\
4
    &start={self.page_number}')
5
            self.soup = BeautifulSoup(self.page.content, 'html.parser')
6
            self.result = self.soup.find(id='resultsCol')

Wystarczy w nawiasach okrągłych dodać po kolei stringi, zostaną one złączone w jeden. Przy tak skomplikowanych elementach powinniśmy przenosić takie argumenty do zmiennych, to żadna oszczędność gdy brak zmiennej generuje brak czytelności.

1
        def get_content(self) -> None:
2
            url = (
3
                'https://pl.indeed.com/jobs?'
4
                f'q={self.job_name}&'
5
                f'l={self.location}&'
6
                'sort=date&'
7
                f'radius={self.radius}&'
8
                f'start={self.page_number}'
9
            )
10
            self.page = self.connect(url)
11
            self.soup = BeautifulSoup(self.page.content, 'html.parser')
12
            self.result = self.soup.find(id='resultsCol')

Niepoprawne zmienne

Nazewnictwo w programowaniu jest istotne. Dlatego też, gdy programista widzi zmienne z prefiksem is_ lub are_ to oczekuje po nich, że znajdzie tam wartość logiczną True lub False. W tym przypadku znajdziemy tam y albo n, co przez Pythona zostanie potraktowane jako True albo True.

1
        def skip(self) -> None:
2
            self.is_skipped = input("Do you want to skip offers that are older\
3
     than 30 days? y/n: ")

Lepiej byłoby od razu przekształcić wartość pobraną od użytkownika na wartość logiczną, co ułatwi następnie korzystanie z tej zmiennej w metodach.

1
        def skip(self) -> None:
2
            self.is_skipped = input("Do you want to skip offers that are older\
3
     than 30 days? y/n: ") == 'y'

Zależności

Klasa wstępnie wygląda na wygodną do użycia w kodzie, jednak jest tutaj zaszyta zależność, a jest nią input.

1
        def skip(self) -> None:
2
            self.is_skipped = input("Do you want to skip offers that are older\
3
     than 30 days? y/n: ")

Jeżeli reszta danych wprowadzana jest przez użytkownika przed utworzeniem obiektu klasy Scraper, to lepiej byłoby pytać go także i o tę wartość, dzięki czemu nasza klasa byłaby łatwiejsza do użycia w testach, i nie byłoby potrzeby mockowania tego elementu podczas testów.

1
    def start_scraping():
2
        job_name = input('Enter job name: ')
3
        place = input('Enter place: ')
4
        radius = int(input('Enter radius: '))
5

6
        scraper = Scraper(job_name, place, radius)
7
        print(f'URL: {scraper.page.url}, Place: {scraper.location}, Job name: \
8
    {scraper.job_name}\n')
9

10
        template = Template(scraper.offers, scraper.number_of_offers)
11

12

13
    if __name__ == '__main__':
14
        start_scraping()

Możemy także pomyśleć o zamianie input na użycie argparse.ArgumentParser, aby móc korzystać z tej aplikacji za pośrednictwem linii poleceń.

Typowanie

Typowanie jest fajne, pomaga IDE szybciej podpowiadać metody, których możemy użyć z daną zmienną, gdyż IDE wie wtedy, czego może od takiej zmiennej oczekiwać. Jednak typy nie są domyślnie sprawdzane podczas działania kodu. I są tu pewne haczyki związane z typowaniem.

Po pierwsze, do typowania kolekcji takich jak dict czy list powinniśmy korzystać z obiektów, które znaleźć można w module typing.

1
        def update_pages(self, pages: int) -> list:
2
            pages_urls = []
3
            for i in range(pages):
4
                counter = i * 10
5
                pages_urls.append(counter)
6
            return pages_urls

Poza tym, jeżeli już typujemy, to musimy robić to poprawnie. Pomijając fakt, że z funkcji nie powinniśmy zwracać różnych typów zmiennych, to w poniższym przypadku typowanie jest niepoprawne, ponieważ zwracamy z funkcji int albo bool. W tym przypadku powinno to być Union[int, bool], ale zdecydowanie lepsze byłoby zwracanie 0 w przypadku gdy nie ma stron, co ma sens logiczny, a dodatkowo zwracamy wtedy już tylko jeden typ zmiennej.

1
        def find_number_of_pages(self) -> int:
2
            pages = self.soup.find(id="searchCountPages")
3
            if not pages:
4
                return False
5
            text = pages.text.strip()
6

7
            # indeed display 15 offers on single page, that's why i divide by 15
8
            number_of_pages = int(re.findall(r"\d+", text)[1])
9
            return math.ceil(number_of_pages / 15)

Gdy funkcja nie zwraca żadnej wartości, to według mnie informacją nadmierną jest pisanie None jako wartości zwracanej. Jest to wartość domyślna, więc nie trzeba o niej informować. Jednak jest to tylko moja opinia.

1
        def skip(self) -> None:
2
            self.is_skipped = input("Do you want to skip offers that are older\
3
     than 30 days? y/n: ")

Ze względu na to, że Python nie korzysta bezpośrednio z typowania, musimy skorzystać z zewnętrznego narzędzia, które typy przez nas wpisane zweryfikuje. Służy do tego narzędzie mypy. W tym przypadku wykryło ono aż 27 niepoprawnie zdefiniowanych typów, i jeżeli chcemy ich używać, zdecydowanie powinniśmy to poprawić.

1
    >> mypy scraper/
2
    ...
3
    scraper/scraper.py:53: error: "None" has no attribute "find_all"
4
    scraper/scraper.py:57: error: "str" has no attribute "text"
5
    scraper/scraper.py:58: error: "str" has no attribute "text"
6
    scraper/scraper.py:59: error: "str" has no attribute "text"
7
    scraper/scraper.py:60: error: "str" has no attribute "text"
8
    scraper/scraper.py:83: error: "object" has no attribute "find"
9
    scraper/scraper.py:90: error: "str" has no attribute "text"
10
    scraper/scraper.py:114: error: Argument 1 to "is_location_set" of "Scraper" has incompatible type "int"; expected "str"
11
    scraper/scraper.py:116: error: Argument 1 to "is_over30_skipped" of "Scraper" has incompatible type "int"; expected "str"
12
    scraper/scraper.py:123: error: Argument 1 to "add_to_offers" of "Scraper" has incompatible type "int"; expected "str"
13
    scraper/scraper.py:123: error: Argument 2 to "add_to_offers" of "Scraper" has incompatible type "int"; expected "str"
14
    scraper/scraper.py:123: error: Argument 3 to "add_to_offers" of "Scraper" has incompatible type "int"; expected "str"
15
    scraper/scraper.py:123: error: Argument 4 to "add_to_offers" of "Scraper" has incompatible type "int"; expected "str"
16
    Found 27 errors in 3 files (checked 6 source files)

Skomplikowane operacje

Kiedy tworzymy funkcje, chcemy robić w nich jak najbardziej atomowe zadania, i robić to jak najczytelniej się da. Jeżeli mamy mieć w funkcji do czynienia ze złożonymi zmiennymi, to warto je odpowiednio przekształcić przed przekazaniem ich do funkcji. Zwłaszcza gdy patrząc na tę funkcję ciężko zrozumieć, dlaczego wywołujemy na zmiennych str dodatkowe operacje.

1
        def add_to_offers(self, title: str, company: str, location: str, date: str,
2
                          link: str) -> None:
3
            self.offers[title.text.strip()] = {
4
                'company': company.text.strip(),
5
                'location': location.text.strip(),
6
                'date': date.text.strip(),
7
                'link': "https://pl.indeed.com" + str(link)
8
            }

Po przekazaniu odpowiednich wartości przy wywołaniu tej metody wyglądałaby ona jak poniżej. Czy nie jest to zdecydowanie czytelniejsze rozwiązanie?

1
        def add_to_offers(self, title: str, company: str, location: str, date: str,
2
                          link: str):
3
            self.offers[title] = {
4
                'company': company,
5
                'location': location,
6
                'date': date,
7
                'link': f"https://pl.indeed.com/{link}"
8
            }

Użycie wbudowanych funkcji

Czasem nie znając możliwości funkcji wbudowanych, próbujemy tworzyć swój własny kod, a wcale nie jest to potrzebne! W tym przypadku tworzymy listę stron z mnożnikiem 10.

1
        def update_pages(self, pages: int) -> list:
2
            pages_urls = []
3
            for i in range(pages):
4
                counter = i * 10
5
                pages_urls.append(counter)
6
            return pages_urls

Możemy to samo osiągnąć, używając wyłącznie funkcji range, która została tutaj użyta. Jeszcze lepiej byłoby zwrócić po prostu range jako generator, lub użyć go bezpośrednio w metodzie. Dodatkowo sama nazwa metody nie jest zgodna z jej funkcjonalnością. Funkcja ta generuje listę kolejnych elementów do wczytania, update informuje, jakoby funkcja ta aktualizowała jakąś zmienną lub pewien zasób, a tak nie jest.

Poniższy przykład poprawy korzysta z list comprehension, aby rezultat był zgodny z typowaniem.

1
        def update_pages(self, pages: int) -> list:
2
            return [i for i in range(0, pages*10, 10)]

Powinniśmy także przenieść tę wartość magiczną jako zmienną, aby opisać, co oznacza ta liczba, na przykład jako offers_per_page.

Pojedyncza odpowiedzialność

Nasze funkcje powinny opierać się na pojedynczej odpowiedzialności. Zwróćmy w pierwszej kolejności uwagę na nazwę metody, która sugeruje, że zwrócimy wartość logiczną. Dzieje się tak, jednak nie bezpośrednio. Co mógłbym tutaj zasugerować? Przekształcenie metody tak, aby zwracać location lub None. None jest i tak traktowany jako False, a location jeżeli nie będzie puste, zostanie potraktowane jako True, więc nie ma sensu zwracać nadmiarowych zmiennych.

1
        def is_location_set(self, location: str, job: object) -> tuple:
2
            if not location:
3
                location = job.find('span', class_='location')
4
                if not location:
5
                    return False, None
6
            return True, location

Poza tym uważam, że z tą funkcją jest wciąż coś nie tak, ale przekształcenie jej mocniej wymagałoby wgryzienia się bardziej w kontekst.

Niepoprawne metody magiczne

Metody magiczne mają swoje konkretne cele, które spełniają w kodzie. To z góry ustalony interfejs. W tej sytuacji programista powinien skorzystać z metody magicznej __str__ w miejsce __repr__ używanego do reprezentacji obiektu w formie tekstowej. Wartość spod __repr__ powinna być możliwa do przetworzenia przez funkcje eval. W poniższym przypadku nie możemy tego oczekiwać.

1
        def __repr__(self) -> str:
2
            return f"Scraper object with {self.get_url()} URL"

Poprawna hermetyzacja

Poprzez używanie metod publicznych, prywatnych oraz chronionych wyznaczamy interfejs dostępny dla użytkownika i klas dziedziczących. W tym przypadku metoda, która wygląda jako główna logika całej klasy, jest ustawiona, jako chroniona (poprzez podwójny znak podkreślenia) co powoduje, że użytkownik nie powinien z niej korzystać, gdyż nie nadaje się ona do użytku publicznego. To błąd, zwłaszcza gdy duża ilość metod w tej klasie nie powinna być dostępna publicznie, a jest. Warto się zastanowić w takich przypadkach, które z metod chcemy, aby użytkownik mógł używać, a które nie. Wiąże się tez z tym kwestia testowania, gdyż powinniśmy testować wyłącznie metody publiczne. Z metod chronionych powinniśmy korzystać w ostateczności!

1
        def __find_job_offers(self) -> None:
2
            self.skip()
3
            pages = self.find_number_of_pages()
4
            if not pages:
5
                print('No offers found! Try to change your query.')
6
                return
7
            ...

Mieszanie poziomów metod

Gdy nasza funkcja zaczyna zajmować dużo linii, możemy być pewni jednego - gdzieś popełnimy błąd. Metoda ta traci na czytelności nie tylko poprzez dużo ukrytych zachowań, ale także przez skomplikowanie w kodzie. Mamy tutaj metody niskiego oraz wysokiego poziomu. Z jednej strony są to metody, które konkretnie opisują zachowanie więc mamy find_number_of_pages i get_content a następnie kod niskiego poziomu, który próbuje wyłuskać nam dane ze strony job.find(...). Powinniśmy taką logikę rozbić na osobne czytelne metody jak process_jobs, get_attributes_from_job_offer. Dodatkowo lepiej jest jawnie przekazywać zmienne między metodami niż robić to pod ukryciem, gdyż w tej logice nie mogę się doszukać, gdzie ukrył się obiekt pobranej strony.

1
        def __find_job_offers(self) -> None:
2
            self.skip()
3
            pages = self.find_number_of_pages()
4
            if not pages:
5
                print('No offers found! Try to change your query.')
6
                return
7

8
            pages_updated = self.update_pages(pages)
9

10
            for page in pages_updated:
11
                self.page_number = page
12
                self.get_content()
13
                jobs = self.find_jobs_div()
14
                for job in jobs:
15
                    title = job.find('h2', class_='title')
16
                    company = job.find('span', class_='company')
17
                    location = job.find('div', class_='location')
18
                    date = job.find('span', class_='date')
19
                    link = job.find('a')['href']
20

21
                    is_location, location = self.is_location_set(location, job)
22

23
                    if not is_location or self.is_over30_skipped(date):
24
                        continue
25

26
                    if None in (title, company, location, date, link):
27
                        print("Something went wrong, offer skipped...")
28
                        continue
29

30
                    self.add_to_offers(title, company, location, date, link)
31
                    self.number_of_offers += 1
32
            print("Done! Go check output.html")

Testy

Aby zacząć pracować nad powyższym kodem, należy napisać testy, które zapewnią nas, że nie popsujemy działającej tam logiki. Dobrym pomysłem także jest taka kompozycja naszego kodu, aby zewnętrzne zależności móc zastąpić adapterem. W tym przypadku pomyślano o “ala” adapterze, czyli odziedziczono po klasie Scraper i nadpisano problematyczne metody. I byłoby to bardzo dobre rozwiązanie, gdyby nie nadpisanie metody __init__ zmieniając jego nagłówek oraz kod w nim wywoływany, a także stworzenie metody find_jobs, która prawdopodobnie miała nadpisać metodę oryginalnej klasy.

1
    from bs4 import BeautifulSoup
2
    import sys
3
    import os.path
4
    sys.path.append(
5
        os.path.abspath(os.path.join(os.path.dirname(__file__), os.path.pardir)))
6
    from scraper import Scraper
7

8

9
    class ScraperLocal(Scraper):
10
        """
11
        Class created only for testing purpose.
12
        """
13
        def __init__(self):
14
            with open('data/sample_page.html', 'r') as f:
15
                self.static_page = f.read()
16
            self.soup = BeautifulSoup(self.static_page, 'html.parser')
17
            self.result = self.soup.find(id='resultsCol')
18
            self.jobs = self.find_jobs_div()
19
            self.offers = {}
20

21
        def find_jobs(self):
22
            for job in self.jobs:
23
                title = job.find('h2', class_='title')
24
                company = job.find('span', class_='company')
25
                location = job.find('div', class_='location')
26

27
                if not location:
28
                    location = job.find('span', class_='location')
29
                    if not location:
30
                        continue
31

32
                date = job.find('span', class_='date')
33
                link = job.find('a')['href']
34

35
                if location is None:
36
                    print("\nSomething went wrong...")
37
                    continue
38
                self.add_to_offers(title, company, location, date, link)
39

40

41
    scraper = ScraperLocal()
42
    scraper.find_jobs()

Reszty testów nie będę opisywać, ponieważ ze względu na skomplikowaną logikę i powyższe nadpisanie głównej logiki klasy - testy niczego konkretnego nie testują.

Uwagi końcowe

Czy to wszystko? Zapewne nie, naprawa tych rzeczy będzie wymagać zmian w wielu miejscach w kodzie, co oznacza, że niektóre elementy zostaną usunięte. Ponieważ ja mogłem coś pominąć, to oznacza, że osoba programująca też mogła nie zauważyć pewnych zależności. To właśnie dlatego dla utrzymania jakości tak ważne jest code review w procesie. Poza tym opisywanie wszystkich bolączek spowodowałoby rozrost i tak już długiego artykułu.

Czy jest to zły kod? Odpowiedź jest jedna - nie. Jest to dobry kod - bo działa, i rozwiązuje problem. To, co można powiedzieć o nim, to że jest nieoszlifowany i brak autorowi pewnego doświadczenia. Gdy się uczymy ciężko o perfekcje, bo jeszcze nie wiemy, co to oznacza, więc musimy się skupić na tym, aby kod działał, a to autorowi udało się w pełni.

Jeżeli potrzebujesz, aby ktoś przyjrzał się Twojemu repozytorium, skontaktuj się z nami!