Dostałem Code Review, robione przez zewnętrzną firmę, z wyszczególnionymi ~20 błędami. Moim zdaniem tylko 2 były sensowne, a reszta wręcz szkodliwa.
Próbowałem się do gościa dobić i przegadać dlaczego uważa tak, a nie inaczej, ale nie miałem okazji. Raz udał mi się wysłać mu wiadomość na discordzie, ale odpowiedział tylko że 'nie pamięta już szczegółów'.
Po 6 miesiącach nadal mnie to swędzi i czuję krytykę z tyłu głowy jak pracuję. Chyba z 50 razy już analizowałem tą listę wzdłuż i wszerz, kupiłem 3 książki na ten temat, a nadal nie jestem pewien który z nas jest upośledzony
Wiem gdzie typ pracuję, myślałem żeby wysłać mu maila z zaproszeniem do przegadania tego, ale nie chcę wyjść na świra.
Kilka dni temu wdrażałem dwa nowe pokemony do projektu (2 regularów) i dałem im im listę tych uwag w formie 'rekrutacji', żeby zobaczyć co by zrobili na moim miejscu.
Odpowiedzi były raczej w moją stronę, ale to jeszcze świeżaki, więc tym bardziej nie wiem czy to dobrze.
Mam zadeklarowaną klasę 'AppColors' z listą kolorów używanych w aplikacji jako stałe statyczne (const static).
Feedback mówi, że AppColors powinno być abstrakcyjnym interfejsem i implementacją wstrzykniętą jako globalny singleton przed DI, żeby zachować odwrócenie zależności i programowanie pod interfejs.
To sobie #!$%@? wymyślił xD prawda jest taka w tym przypadku nie ma to absolutnie żadnego znaczenia bo na litość boską to jest po prostu jakiś prosty zbiór wartości który chcesz
Żeby teraz dodać ten feature to będziesz musiał przeorać kod.
@gos4k: I co z tego? Tylko że w tym czasie wersja bez zmienialnych kolorów już będzie na rynku i sfinansuje mi studenta który przeora kod, a Ty nadal będziesz rzeźbił wieżę z kości słoniowej i w końcu utoniesz w tysiącach linii niepotrzebnego kodu. A potem zresztą bardzo możliwe że i tak będziesz musiał przeorać kod, bo stosowanie OCP wcale nie gwarantuje,
@Krolik: no właśnie dlatego usunąłem komentarz ale zdążyłeś odpowiedzieć.
Pisanie kodu łatwego w rozszerzaniu to nie jest rzeźbienie wieży z kości słoniowej i tysiące linii niepotrzebnego kodu a interfejs to nie abstrakcja. Ofc wrzucanie interfejsu wszędzie to głupota ale skoro ktoś dał takie review to zwiększa szanse że faktycznie tak powinno być
@Krolik: no architektura i zalozenia nie sa zero jedynkowe, ze albo wszystko piszemy rozszerzalne albo wszystko piszemy "z palca i na zawsze" i nie ma nic pomiedzy.
To kwestia wywazenia, wyczucia domeny, intuicji nabytej z doswiadczeniem, potrzeb klienta, umiejetnosci, zespolu, zasobow czasu i wielu, wielu innych kwestii.
@Reevo: kto zleca takie zewnętrzne peer review? PM? Nigdy o czymś takim nie słyszałem. Rozumiem takie rzeczy jak chodzi o security, ale jak chodzi o kod to bym chyba jebnął papierami. Wiadomo, że z zewnątrz znajdzie gówno problemy, często filozoficzne i nawet nie podyskutujesz ze swoim managerem, bo wyjdzie, że pewnie się tłumaczysz z błędów.
@Reevo: kto zleca takie zewnętrzne peer review? PM? Nigdy o czymś takim nie słyszałem. Rozumiem takie rzeczy jak chodzi o security, ale jak chodzi o kod to bym chyba jebnął papierami. Wiadomo, że z zewnątrz znajdzie gówno problemy, często filozoficzne i nawet nie podyskutujesz ze swoim managerem, bo wyjdzie, że pewnie się tłumaczysz z błędów.
@WyjmijKija: To była firma która outsourcowała nam przez jakiś czas projekt. Na koniec robiliśmy sobie
Mam zadeklarowaną klasę 'AppColors' z listą kolorów używanych w aplikacji jako stałe statyczne (const static).
Feedback mówi, że AppColors powinno być abstrakcyjnym interfejsem i implementacją wstrzykniętą jako globalny singleton przed DI, żeby zachować odwrócenie zależności i programowanie pod interfejs.
ale to wygląda jak downgrade. #!$%@? typa, nie zawsze to, co w review jest lepsze. singleton w tym przypadku nie jest absolutnie potrzebny, a same singletony nie cieszą się dobrą sławą
@Reevo: Kiedy termin jest na wczoraj, bo się pali, to wszelakie zasady często idą w #!$%@?. Jak mi na studiach doktor powiedział jak oddawałem program: "dobrze, ale ja bym zrobił inaczej. 95/100 pkt"
singletony nie cieszą się dobrą sławą przecież, coś jak antypattern
@314159: ludzie marudzą bo gdzieś tak usłyszeli i bezmyślnie powtarzają, jednocześnie korzystają i często nawet nie zdają sobie z tego sprawy. ¯\_(ツ)_/¯
Singleton to standardowa praktyka jeżeli masz jakiś stateless service, po jaką cholerę dependency container miałby ci wypluwać za każdym razem nową instancję? Nie jest to może klasyczna implementacja z książki GoF, ale funkcjonalnie na jedno wychodzi. Nie wiem jak
Nie jest to może klasyczna implementacja z książki GoF, ale funkcjonalnie na jedno wychodzi.
@croppz: to jest akurat oczywiste, problem jest w zależnościach pojawiających się nagle gdzieś w środku kodu i ogólnej upierdliwości w testowaniu tego. W przypadku klasycznego DI widzisz dokładnie co masz na wejściu i możesz to łatwo podmienić. Pół biedy jak znasz dobrze kod i wiesz czego się spodziewać, ale singletony w czymś nowym to jest koszmar.
problem jest w zależnościach pojawiających się nagle gdzieś w środku kodu i ogólnej upierdliwości w testowaniu tego. W przypadku klasycznego DI widzisz dokładnie co masz na wejściu i możesz to łatwo podmienić.
@boryspo: mógłbyś trochę doprecyzować o co chodziło? Bo nie jestem pewien czy rozumiem.
"Klasyczne DI" rozumiem właśnie jako dependency container wstrzykujący w konstruktor odpowiednią klasę na podstawie typu parametru (czy tam interfejsu, jeżeli w konfiguracji zmapujemy ten interfejs na
@Reevo: firma zewnętrzna wzięła kasę za review, więc do czegoś #!$%@? się musieli, bo gdyby napisali, że wszystko jest super i nie zgłosili żadnych uwag to w oczach zleceniodawcy mogliby wyglądać nieprofesjonalnie, a Ty od pół roku nie śpisz z tego powodu. Stary, zluzuj majty i rób swoje, a review jakiegoś randoma się nie przejmuj.
@Reevo: Twój wpis brzmi jak bait/pasta. Jeśli jest na serio to po prostu wyluzuj. W review nie zawsze komentarze są sensowne. Recenzent tez może się mylić. Nie analizował bym reviews sprzed pół roku jak pisma świętego, albo innej prawdy objawionej. W Twojej ocenie tylko 2 komentarze były sensowne i prawdopodobnie tak właśnie było.
Aktywne Wpisy
18+
Zawiera treści 18+
Ta treść została oznaczona jako materiał kontrowersyjny lub dla dorosłych.
Próbowałem się do gościa dobić i przegadać dlaczego uważa tak, a nie inaczej, ale nie miałem okazji. Raz udał mi się wysłać mu wiadomość na discordzie, ale odpowiedział tylko że 'nie pamięta już szczegółów'.
Po 6 miesiącach nadal mnie to swędzi i czuję krytykę z tyłu głowy jak pracuję. Chyba z 50 razy już analizowałem tą listę wzdłuż i wszerz, kupiłem 3 książki na ten temat, a nadal nie jestem pewien który z nas jest upośledzony
Wiem gdzie typ pracuję, myślałem żeby wysłać mu maila z zaproszeniem do przegadania tego, ale nie chcę wyjść na świra.
Kilka dni temu wdrażałem dwa nowe pokemony do projektu (2 regularów) i dałem im im listę tych uwag w formie 'rekrutacji', żeby zobaczyć co by zrobili na moim miejscu.
Odpowiedzi były raczej w moją stronę, ale to jeszcze świeżaki, więc tym bardziej nie wiem czy to dobrze.
Panie doktorze, ego mnie boli (╥﹏╥)
#programowanie #programista15k
Komentarz usunięty przez autora
To sobie #!$%@? wymyślił xD prawda jest taka w tym przypadku nie ma to absolutnie żadnego znaczenia bo na litość boską to jest po prostu jakiś prosty zbiór wartości który chcesz
Komentarz usunięty przez autora
@gos4k: I co z tego? Tylko że w tym czasie wersja bez zmienialnych kolorów już będzie na rynku i sfinansuje mi studenta który przeora kod, a Ty nadal będziesz rzeźbił wieżę z kości słoniowej i w końcu utoniesz w tysiącach linii niepotrzebnego kodu. A potem zresztą bardzo możliwe że i tak będziesz musiał przeorać kod, bo stosowanie OCP wcale nie gwarantuje,
Pisanie kodu łatwego w rozszerzaniu to nie jest rzeźbienie wieży z kości słoniowej i tysiące linii niepotrzebnego kodu a interfejs to nie abstrakcja. Ofc wrzucanie interfejsu wszędzie to głupota ale skoro ktoś dał takie review to zwiększa szanse że faktycznie tak powinno być
To kwestia wywazenia, wyczucia domeny, intuicji nabytej z doswiadczeniem, potrzeb klienta, umiejetnosci, zespolu, zasobow czasu i wielu, wielu innych kwestii.
Nie ma uniwersalnych odpowiedzi.
@Reevo: o #!$%@?, co to za rak xD
@WyjmijKija: To była firma która outsourcowała nam przez jakiś czas projekt. Na koniec robiliśmy sobie
Komentarz usunięty przez autora
Mam zadeklarowaną klasę 'AppColors' z listą kolorów używanych w aplikacji jako stałe statyczne (const static).
Feedback mówi, że AppColors powinno być abstrakcyjnym interfejsem i implementacją wstrzykniętą jako globalny singleton przed DI, żeby zachować odwrócenie zależności i programowanie pod interfejs.
ale to wygląda jak downgrade. #!$%@? typa, nie zawsze to, co w review jest lepsze. singleton w tym przypadku nie jest absolutnie potrzebny, a same singletony nie cieszą się dobrą sławą
Jak mi na studiach doktor powiedział jak oddawałem program: "dobrze, ale ja bym zrobił inaczej. 95/100 pkt"
@314159: ludzie marudzą bo gdzieś tak usłyszeli i bezmyślnie powtarzają, jednocześnie korzystają i często nawet nie zdają sobie z tego sprawy. ¯\_(ツ)_/¯
Singleton to standardowa praktyka jeżeli masz jakiś stateless service, po jaką cholerę dependency container miałby ci wypluwać za każdym razem nową instancję? Nie jest to może klasyczna implementacja z książki GoF, ale funkcjonalnie na jedno wychodzi. Nie wiem jak
@croppz: to jest akurat oczywiste, problem jest w zależnościach pojawiających się nagle gdzieś w środku kodu i ogólnej upierdliwości w testowaniu tego. W przypadku klasycznego DI widzisz dokładnie co masz na wejściu i możesz to łatwo podmienić. Pół biedy jak znasz dobrze kod i wiesz czego się spodziewać, ale singletony w czymś nowym to jest koszmar.
@boryspo: mógłbyś trochę doprecyzować o co chodziło? Bo nie jestem pewien czy rozumiem.
"Klasyczne DI" rozumiem właśnie jako dependency container wstrzykujący w konstruktor odpowiednią klasę na podstawie typu parametru (czy tam interfejsu, jeżeli w konfiguracji zmapujemy ten interfejs na