Roast me! Czyli co byście zmienili w mojej solucji?

Hej, napisałem sobie taki pół-żartem, pół-serio program do typowania koni w wyścigach. Nie chodzi mi tu o ocenę funkcjonalności czy skutecznośi, a architektury i błędów w sztuce. Ciekaw jestem ile uda Wam się ich znaleźć. Na chwilę obecną głównie się obawiam, że mam zbyt wiele logiki w warstwie serwisowej i jeszcze nie wiem jak ugryźć testowanie serwisów. TUTAJ też znajduje się cała logika dla pobierania danych. Do tego mogłem z niektórych metod utworzyć oddzielne serwisy, ale tutaj ciekaw jestem czy Wy wskażecie z których.

Solucja wykorzystuje m.in. Autofac, HtmlAgilityPack, Wpf.Toolkit, Newtonsoft.Json, xUnit, Moq. Jest też sporo asynchronicznych metod.

Dopiero zacząłem pisać testy jednostkowe do niej, więc jeszcze wszystkiego nimi nie pokryłem. Sporo czasu minie nim je skończę, bo 1) do tej pory rzadko pisałem testy, pora się nauczyć je pisać 2) testowanie asynchronicznego kodu nie jest łatwe, a tego mam tu sporo. Też pewnie będę ciągle refaktoryzował SUT pod testowanie, ale jak coś dziwnego zauważycie w kodzie to piszcie śmiało. Z góry dzięki za poświęcony czas i wszelkie uwagi.

GitHub link: https://github.com/przemyslawbak/Horse_Picker

A poniżej screen UI:

Ubogie modele i ViewModel na 2k linii? No nie wiem czy w dobrą stronę idziesz.

Pierwsze co widzę do poprawy to UI – paskudna kolorystyka, ten zielony nie pasuje do niczego. Najlepiej to ustaw domyślne, systemowe kolory, tak aby Twoja plikacja wyglądała tak jak reszta systemu.

A jeśli już koniecznie chcesz mieć własną kolorystykę to skorzystaj z narzędzi do doboru kolorów i odcieni, tak aby interfejs nie wypalał oczu. Ewentualnie zamiast jednokolorowego tła, skorzystaj z czegoś ciekawszego – może gradient (ale nie taki prosty wertykalny), może tekstura (byle nie samopowtarzająca).

Użytego języka nie znam, więc nie skomentuję.

No cóż, nigdy nie pisałem w WPF, ale jak na moje oko tutaj żadnej architektury nie ma i większość jest do poprawy.

3 serwisy na krzyż z czego jeden na ponad 1k linii i metody na 200+ linii i dodatkowo 1 viewmodel który ma 2k linii. Tam siedzi cała logika.

A co do tego użycia async to owszem, używasz, ale w sposób jaki się nie powinno używać bo to co robisz to anti-pattern. W FileDataServices i ScrapDataServices masz metody, które korzystają z I/O (odczyt/zapis z dysku i wysyłanie requestów http), a zamiast użyć asynchronicznych text readerów, czy httpclienta to wszystko robisz synchronicznie i wrapujesz to w Task.Run.

https://blog.stephencleary.com/2013/11/taskrun-etiquette-examples-dont-use.html

furious programming napisał(a):

Pierwsze co widzę do poprawy to UI (...)

Poprawiłem trochę kolory, myślisz że już lepiej?

Dzięki za ciekawe uwagi.

some_ONE napisał(a):

W FileDataServices i ScrapDataServices masz metody, które korzystają z I/O (odczyt/zapis z dysku i wysyłanie requestów http), a zamiast użyć asynchronicznych text readerów (...)

Co do FileDataServices, odczyt z plików nie trwa długo, nie stanowi to najmniejszego problemu. Rozważałem asynchroniczny zapis, ale ostatecznie zdecydowałem się zostać przy synchronicznym. Czy to jakiś problem? Choć może dla dobrej praktyki zrobię asynchroniczny odczyt/zapis. Choć Newtonsoft.Json nie wspiera asynchronicznej (de)serializacji, może zastosuję wspominany wcześniej antypattern żeby nie blokować UI: https://stackoverflow.com/a/15648126 .

A w TextWriter zastosowałem już asynchroniczny WriteLineAsync:

async Task SaveTestResultLine(string line)
        {
            using (TextWriter csvLineBuilder = new StreamWriter(_testsFileName, true))
            {
                await csvLineBuilder.WriteLineAsync(line);
            }
        }

some_ONE napisał(a):

wszystko robisz synchronicznie i wrapujesz to w Task.Run.

https://blog.stephencleary.com/2013/11/taskrun-etiquette-examples-dont-use.html

Wczytałem się w ten artykuł. Jak dobrze rozumiem, konkluzja jest taka, że UI powinno dbać o nieblokowanie wątku UI i nie powinno się tworzyć nowych wątków które mogą wpłynąć na pogorszenie wydajności. Jestem skłonny się zgodzić, ale w moim przypadku Task task = Task.Run(async () => ma na celu właśnie utworzenie przez pętlę for jak największej ilości Task-ów i wątków, które będą pracować równolegle dla przyspieszenia pracy z racji długiego trwania wykonywania każdego z zadania. Mi właśnie zależy na utworzeniu wielu nowych wątków i nie znalazłem lepszego rozwiązania. Ewentualnie Parallel.For mogłoby być alternatywą.

some_ONE napisał(a):

3 serwisy na krzyż z czego jeden na ponad 1k linii i metody na 200+ linii i dodatkowo 1 viewmodel który ma 2k linii. Tam siedzi cała logika.

Tutaj się zgodzę w 100%. Narobiłem trochę ciężkich metod i klas, pomyślę o ich odchudzeniu i zrobieniu z niektórych metod nowych serwisów.

bakunet napisał(a):

Co do FileDataServices, odczyt z plików nie trwa długo, nie stanowi to najmniejszego problemu. Rozważałem asynchroniczny zapis, ale ostatecznie zdecydowałem się zostać przy synchronicznym. Czy to jakiś problem?

No nie trwa zbyt długo i w aplikacji desktopowej to raczej nie ma dużego znaczenia, ale dla przyzwoitości można napisać tak jak poprawnie powinno być :P
W apce webowej teoretycznie mogłoby to mieć znaczenie, bo tam możesz dostać nagle 10k requestów na sekundę i przy synchronicznym zapisie mogą się skończyć dostępne wątki.

Choć może dla dobrej praktyki zrobię asynchroniczny odczyt/zapis. Choć Newtonsoft.Json nie wspiera asynchronicznej (de)serializacji.

A nie możesz serializować w pamięci przez JsonConvert, a później asynchronicznie zapisać do pliku?

Wczytałem się w ten artykuł. Jak dobrze rozumiem, konkluzja jest taka, że UI powinno dbać o nieblokowanie wątku UI i nie powinno się tworzyć nowych wątków które mogą wpłynąć na pogorszenie wydajności. Jestem skłonny się zgodzić, ale w moim przypadku Task task = Task.Run(async () => ma na celu właśnie utworzenie przez pętlę for jak największej ilości Task-ów i wątków, które będą pracować równolegle dla przyspieszenia pracy z racji długiego trwania wykonywania każdego z zadania. Mi właśnie zależy na utworzeniu wielu nowych wątków i nie znalazłem lepszego rozwiązania. Ewentualnie Parallel.For mogłoby być alternatywą.

W poprzednim poście pisałem tylko o ScrapDataServices i FileDataServices, tam też w kilku miejscach wrzucasz operacje synchroniczne w Task.Run, a nie powinno to tam być.

A co do przykładu o którym mówisz z MainViewModel to też wcale Task.Run tam nie jest potrzebne, bo każdą operację asynchroniczną opakowujesz w Task.Run, co znowu nie jest dobrą praktyką. Ten kod powinien wyglądać mniej więcej tak (pisane z palca, więc pewnie są jakieś błędy, ale ogólną ideę powinno być widać):

[...]
List<Task> tasks = Enumerable.Range(startIndex, stopIndex - startIndex)
   .Select(i => ScrapData(i))
   .ToList();

await Task.WhenAll(tasks); //albo WhenAny w pętli i tutaj aktualizujesz ProgressBara

[...]

private async Task ScrapData(int index)
{
    if (dataType == "racesPl") race = await _scrapServices.ScrapSingleRacePlAsync(index);

    //if the race is from 2018
    if (race.RaceDate.Year == 2018)
    {
        lock (((ICollection)Races).SyncRoot)
        {
            Races.Add(race);
        }
    }
}

Tutaj się zgodzę w 100%. Narobiłem trochę ciężkich metod i klas, pomyślę o ich odchudzeniu i zrobieniu z niektórych metod nowych serwisów.

Ja bym zaczął od wydzielenia klasy, która będzie odpowiadała za komunikację z tym serwisem z którego scrapujesz dane (teraz ładujesz stronę przez htmlagilitypack, a z tego co widziałem on nie obsługuje asynca). Ja bym spróbował zrobić klasę, która pobierze zawartość strony jako HTML przez HttpClienta (ma asynca) i dopiero później ładował gotowego stringa do HtmlAgilityPack (chyba się da?).

Dodatkowo przydałoby się wydzielić to całe parsowanie przez HtmlAgilityPack do jakiejś oddzielnej klasy.

No i jeszcze w FileDataServices masz kilka metod, które robią to samo (czytanie z pliku -> deserializacja, albo serializacja -> zapis do pliku). I teraz w każdej metodzie masz zrobiony copy-paste. Zrób sobie na początek jakiś generyczny helper wewnątrz tej klasy i te 3 metody GetAllHorses/GetAllJockeys/GetAllRaces to będą tylko wywołania helpera.

Odnośnie kolorów zoba taki szajs:
https://material.io/tools/color/#!/?view.left=0&view.right=0&primary.color=7E57C2
https://www.materialpalette.com/
http://mcg.mbitson.com/#!?mcgpalette0=%233f51b5&themename=mcgtheme

some_ONE napisał(a):

A co do przykładu o którym mówisz z MainViewModel to też wcale Task.Run tam nie jest potrzebne, bo każdą operację asynchroniczną opakowujesz w Task.Run, co znowu nie jest dobrą praktyką.

Tutaj się nie zgodzę, bo w razie potrzeby potrzebuję uśmiercić wszystkie zadania w poprawny sposób. Tak więc Przez Task.Run mogę wykorzystać CancellationToken jak w ODPOWIEDZI NA TO ZAPYTANIE.

some_ONE napisał(a):

Ja bym zaczął od wydzielenia klasy, która będzie odpowiadała za komunikację z tym serwisem z którego scrapujesz dane (teraz ładujesz stronę przez htmlagilitypack, a z tego co widziałem on nie obsługuje asynca). Ja bym spróbował zrobić klasę, która pobierze zawartość strony jako HTML przez HttpClienta (ma asynca) i dopiero później ładował gotowego stringa do HtmlAgilityPack (chyba się da?).

Dodatkowo przydałoby się wydzielić to całe parsowanie przez HtmlAgilityPack do jakiejś oddzielnej klasy.

No i jeszcze w FileDataServices masz kilka metod, które robią to samo (czytanie z pliku -> deserializacja, albo serializacja -> zapis do pliku). I teraz w każdej metodzie masz zrobiony copy-paste. Zrób sobie na początek jakiś generyczny helper wewnątrz tej klasy i te 3 metody GetAllHorses/GetAllJockeys/GetAllRaces to będą tylko wywołania helpera.

Zastosuję się do Twoich rad. Czy do tych klas robić interfejsy, żeby łatwiej było testować? Ciągle też się zastanawiam jak wygląda testowanie serwisu, więc nie wykluczam, że będę miał dalsze pytania, jak nie uda mi się znaleźć ciekawych przykładów ;p

Na szczeście da się parsować dokument pobrany przez HttpClienta z HtmlAgilityPack, już kiedyś miałem okazję to robić.

Co do FileDataServices to się zgadza, myslałem o optymalizacji tych metod, dokładałem je wraz z rozwojem VM i zmianą wykorzystanych bibliotek.

some_ONE napisał(a):

W poprzednim poście pisałem tylko o ScrapDataServices i FileDataServices, tam też w kilku miejscach wrzucasz operacje synchroniczne w Task.Run, a nie powinno to tam być.

Co do ScrapDataServices to się nie zgodzę z Tobą. Nawiązując do TEGO ARTYKUŁU i TEJ ODPOWIEDZI wydaje mi się, że wykonałem wszystko zgodnie ze sztuką, ponieważ:

1. metoda wywołująca (ScrapHorses w VM) blokujące zadanie (ScrapSingleHorsePlAsync) jest async;
2. wywołanie jest await
3. blokujące zadanie (ScrapSingleHorsePlAsync) jest async

Przykład z artykułu:

class MyService
{
  public async Task<int> RetrieveValueAsync(int id)
  {
    // Converted to nonblocking work.
    // DB access, web request, etc.
    await Task.Delay(500);
    return 42;
  }
}

some_ONE napisał(a):

(WSZYSTKO)

Ok, muszę przyznać Ci całkowitą rację co do mojego wykorzystania Task.Run :) Na SO zdmuchnęli resztkę moich nadziei na to, że może jednak jest OK. Potrzebuję teraz przebudować swoje serwisy i VM, chwilę pewnie mi to zajmie. Ale przynajmniej długo nie zapomnę tej lekcji o Taskach i Runach :)

Tak więc dzięki raz jeszcze za zwrócenie uwagi!

Liczby masz przygotowane z trzema miejscami po przecinku. Nie lepiej wyrównać je do prawej strony?