search

WitoldSurowka / biker_info_jsonDB

a service to inform bikers of coming weather conditions
0 stars 0 forks source link

Code Review #8

Open maciekmm opened 3 months ago

maciekmm commented 3 months ago

Cześć!

Jeśli dobrze rozumiem, to chcesz albo co minutę albo raz na dzień w okolicach 20:22:50 wysłać pogodę SMSem do odbiorców.

Ogólnie dobra robota, fajnie, że to działa. Jest sporo kwestii, które można poprawić, ale jest całkiem neźle.

Zaczynając od początku - polecam napisać testy do całości. Wiem, że testy nie są sexy, ale wymuszają niezłą strukturę kodu. Pokażą Ci, gdzie trzeba wprowadzić jakiś refactoring.

Diagram w README jest dość nieczytelny, nie wiem co dokładnie chcesz przekazać :) logger tym przypadku jest szcegółem implementacyjnym, też te zielone obiekty na górze nie wiadomo co robią. Myślę, że warto sobie poczytać o https://c4model.com/ albo nawet o diagramach UMLowych.

Uruchom sobie na swoim kodzie takie narzędzia jak staticcheck https://github.com/dominikh/go-tools#installation czy semgrep. Pokażą Ci sporo problemów i raczej mało false-positiveów.

Przeczytaj https://go.dev/doc/effective_go Zrób https://go.dev/tour/welcome/1 :)

Dodaj .idea/ do .gitignore.

Przechodząc do samego kodu.

main.go

https://github.com/WitoldSurowka/biker_info/blob/deed92926752075fe3053587eb8d6770a406c680/main.go#L9-L33

Użycie time.Ticker w tym wydaniu jest dość problematyczne, Nie masz gwarancji, że time.Ticker uruchomi się akurat w 50. sekundzie. Ponadto na channel time.Ticker wrzuca już obecny time.Time, więc nie musisz wołać time.Now(). Najlepiej by chyba było użyć time.NewTimer(duration), gdzie duration do kolejnego wywołania wyliczysz ręcznie. Po każdym wywołaniu możesz zawołać Timer.Reset(duration), gdzie duration jest liczone ponownie.

Staraj się też trzymać zmienne blisko miejsca użycia, time.Now() na samej górze wisi dość daleko od miejsca użycia i w sumie nie jest potrzebne w ogóle.

API.go

https://github.com/WitoldSurowka/biker_info/blob/deed92926752075fe3053587eb8d6770a406c680/API.go#L8-L9 https://github.com/WitoldSurowka/biker_info/blob/deed92926752075fe3053587eb8d6770a406c680/API.go#L38-L40

Inicjalizujesz repozytorium wiele razy w trakcie wykonania. Lepszym pomysłem byłoby otworzyć repo w main() i przekazywać je dalej. Najlepiej opakować w jakiegoś structa - często jest to coś w stylu:

type App struct {
   repo Repository
}

https://github.com/WitoldSurowka/biker_info/blob/deed92926752075fe3053587eb8d6770a406c680/API.go#L13-L24 Tu można switcha użyć: https://go.dev/wiki/Switch

    switch {
    case content == "help":
        InfoHelp(phoneNumber)
    case content == "miasta":
        //...
    case content == "rodo":
        //...
    case CheckIfCityInBase(content):
        //...
    case !CheckIfCityInBase(content):
        //...
    }

https://github.com/WitoldSurowka/biker_info/blob/deed92926752075fe3053587eb8d6770a406c680/API.go#L39-L41 Nie ignoruj errorów, przekaż je dalej i obsłuż w miejscu wywołania, a przynajmniej zaloguj.

https://github.com/WitoldSurowka/biker_info/blob/deed92926752075fe3053587eb8d6770a406c680/API.go#L61-L65 Możesz użyć multiline stringów https://stackoverflow.com/questions/7933460/how-do-you-write-multiline-strings-in-go

DBManager.go

Community wokół Go jest raczej uczulona na słowa typu Manager ;) można to lepiej nazwać.

https://github.com/WitoldSurowka/biker_info/blob/deed92926752075fe3053587eb8d6770a406c680/DBmanager.go#L12-L31

  1. Ignorujesz errory przy operacjach.
  2. Troszkę mieszasz odpowiedzialności, DBManager nie powinien sam wysyłać powiadomień (e.g. InfoStatusAdded(phoneNumber, city)) Zrób to w miejscu w którym wołasz AddRecord.

Całkiem fajnie poza tym to wygląda. Warto się też nauczyć SQLa, więc możesz taką bazkę postawić na sqlite na przykład zamiast samemu implementować.

models.go

https://github.com/WitoldSurowka/biker_info/blob/deed92926752075fe3053587eb8d6770a406c680/models.go#L33-L35

Co tu się podziało? :) Najlepiej poswitchuj po numerze dnia zamiast nazwie, nie będziesz musiał się bawić w formatowanie.

https://github.com/WitoldSurowka/biker_info/blob/deed92926752075fe3053587eb8d6770a406c680/models.go#L49-L55

Zamiast synonimów poszedłbym w jakąś normalizację, albo najlepiej to użył API do geolokalizacji jeśli jakieś znajdziesz.

WeatherFetcher

Yr.no ma API :) Zdecydowanie lepiej go użyć niż scrapeować stronę, aczkolwiek scrapeowanie to też dobre ćwiczenie. https://developer.yr.no/doc/GettingStarted/

https://github.com/WitoldSurowka/biker_info/blob/deed92926752075fe3053587eb8d6770a406c680/weatherFetcher.go#L48-L50

Takie slice accessy są dość niebezpieczne, jak coś będzie mieć mniej/więcej znaków niż oczekujesz to zacznie ci się wszystko krzaczyć. Rób checki czy dane mają taką długość jakiej się spodziewasz.

WitoldSurowka commented 3 months ago

Dzięki za porządny Code Review i miłe słowo :) Jeśli dobrze rozumiem, to chcesz albo co minutę albo raz na dzień w okolicach 20:22:50 wysłać pogodę SMSem do odbiorców. Dokładnie. Docelowo chcę wysyłać feed co wieczór. W repozytorium jest warunek na 50. sekundę, ale to na potrzeby testowania manualnego. time.NewTimer(duration) to z pewnością lepszy (bezpieczniejszy) pomysł, aczkolwiek bazując na dotychczasowych doświadczeniach ticker nie pominał mi nigdy oczekiwanej wartości.

Testy z pewnością napiszę, bo to obszar GO, który akurat chcę poznać.

Przeczytaj https://go.dev/doc/effective_go - przestudiuję:) Zrób https://go.dev/tour/welcome/1 :) - to przeszedłem ze dwa razy :)

Inicjalizujesz repozytorium wiele razy w trakcie wykonania. Lepszym pomysłem byłoby otworzyć repo w main() i przekazywać je dalej.... - czy to nie będzie problematyczne jeżeli maszyna na której to będzie zdeployowane będzie ulegała wyłączeniom? Zamierzam postawić to na małym bare metalu, w domu.

Tu można switcha użyć: https://go.dev/wiki/Switch - no pewnie....

Nie ignoruj errorów, przekaż je dalej i obsłuż w miejscu wywołania, a przynajmniej zaloguj. - ok ok

Troszkę mieszasz odpowiedzialności, DBManager nie powinien sam wysyłać powiadomień (e.g. InfoStatusAdded(phoneNumber, city)) Zrób to w miejscu w którym wołasz AddRecord - dobra uwaga. Chcę żeby ten kod był SOLID!

Warto się też nauczyć SQLa - znam, znam, po prostu uznałem, że nie warto 'strzelać z armaty do wróbli' i json wystarczy w tym przypadku, choć pewnie mało to rozwojowe w proównaniu z jakąś relational database.

Yr.no ma API :) - rzeczywiście! Początkowo chciałem zrobić integrację z API ICM UW Meteo, ale jest płatne... No przynajmniej poćwiczyłem web scraping :D