Przekonywanie członków zespołu do nowych pomysłów

0

Hej,
Jeżeli temat nie pasuje do działu to proszę o przeniesienie, mam do was następujące pytania:

  1. Jak przekonać zespół, w którym się pracuje do tego że mutowalność jest zła? Chodzi mi o to że prywatnie mam już za sobą etap w życiu gdy pisałem voidową metodę z 5 parametrami, gdzie dwa z nich były modyfikowane a jeden był switchem i wiem że katastrofalnie się to kończyło gdy po paru miesiącach trzeba było „doklepać mały ficzer” w takim miejscu. W obecnej pracy praktyka ta wydaje się być dość powszechna bo można „dwie pieczenie na jednym ogniu i przy okazji, oszczędność w kodzie, bo im mniej kodu tym lepiej”. Jakie argumenty można przedstawić, prace, artykuły, dowody? Nie mam dość autorytetu ze względu na staż pracy w obecnym miejscu przez co powołanie się na kogoś „z góry” byłoby ok. Ten sam problem tyczy się obiektów DTO gdzie w swoim cyklu życia zmienia ono stan kilka razy poprzez setery.

  2. Drugie pytanie jakie mam: Jak wygląda u was pseudokod w kodzie produkcyjnym? Zostawiacie komentarze w kodzie czy praktykujecie zasadę „żadnych komentarzy”, według mnie zostawienie czegoś takiego

// oznacz fakture do rozliczenia
Faktura faktura = repo.getFak(...)
Oznaczenie oznaczenia = slownikService.get(..magiczny numer...);
faktura.setOznaczenie(oznaczenia);
// wyszukuje z tabeli STML po rekordach z tabeli OZN_OLD po kombinacji pól dat na czas w wykonania metody, 
// nikt nic nie wie, wszyscy co to pisali się zwolnili, szukaj w JIRA-0101
LocalDate dataStemplacji = serwis.daty();
STMP stmp = repo.findBy(dataStemplacji, faktura.getOznaczenieGlowne().getblablabla pociąg getów);
magicznyserwis.execute(stmp, faktura); // to jest void ktory robi 4 rzeczy rownolegle

Jest przydatne, bo objaśnia dlaczego tak a nie inaczej i co się dzieje, niestety bardzo często przez naleciałości z danych lat powodują że czynność A wymaga zupełnie nieoczywistej zmianie w bazie danych – nic nie mówiące nazwy tabel albo tabele bóstwa gdzie jest wszystko. A no i serwisy są robione po generykach z dziedziczeniem.

  1. Jak wygląda wasza relacja między kodem a diagramem np. BPMN z dokumentacji analityka? Do tej pory forsowałem podejście że jednak lepiej trzymać się blisko dokumentacji z tego względu że wdrożenie nowej osoby do projektu jest łatwiejsze, na zasadzie: „tu masz schemat, tu masz metodę serwisową, obczaj sobie, jest prawie jeden do jeden”. Argument za tym jest taki że utrzymanie tego kodu jest według mnie łatwiejsze, niestety spotykam się często z komentarzami „to za dużo kodu, można to zrobić sprytniej np. voidową metodą z 5 argumentami”.

Jakie jest wasze zdanie na ten temat? Może ja się mylę? Zapraszam do dyskusji. ;) Szczególnie liczę na @jarekr000000 i @Shalom Z góry dzięki.

1

Ad.1 Byłem kiedyś w takim zespole. Żadne autorytety ich nie przekonają, pokazane prezentacje, case'y. Uważam, że ty miałbyś większą siłę przebicia! Tylko zajmie to sporo czasu. Ale kropla drąży skałę. To jest bardzo częsty problem kiedy zespół ma narzucony styl (często badziewny tak jak tutaj), nierzadko przez kogoś kto wyrobił sobie złe nawyki w pisaniu kodu i jakakolwiek próba podważenia jego wiedzy będzie się kończyć źle ...szczególnie dla Ciebie. W takich przypadkach nie wiem czy nie pomogły by wprost pokazanie ile błędów pojawia się wokół takiego kodu, ile czasu spędza się na jego znalezienie, debugowanie itd. Ale to trzeba mierzyć, wprowadzić metryki. Temat niełatwy i większość uzna, że niepotrzebny. I tak to szaleństwo będzie trwać nadal :)

Ad. 2 Gdyby te nazwy metod były bardziej znaczące i tak jak piszesz metoda nie wykonywała by N rzeczy na raz to żaden taki komentarz nie byłby potrzebny. Co to jest serwis.daty() ?! Wywołałeś u mnie złe wspomnienia ;)

0
Grzegorz Kotfis napisał(a):

Ad.1 Byłem kiedyś w takim zespole. Żadne autorytety ich nie przekonają, pokazane prezentacje, case'y. Uważam, że ty miałbyś większą siłę przebicia! Tylko zajmie to sporo czasu. Ale kropla drąży skałę. To jest bardzo częsty problem kiedy zespół ma narzucony styl (często badziewny tak jak tutaj), nierzadko przez kogoś kto wyrobił sobie złe nawyki w pisaniu kodu i jakakolwiek próba podważenia jego wiedzy będzie się kończyć źle ...szczególnie dla Ciebie. W takich przypadkach nie wiem czy nie pomogły by wprost pokazanie ile błędów pojawia się wokół takiego kodu, ile czasu spędza się na jego znalezienie, debugowanie itd. Ale to trzeba mierzyć, wprowadzić metryki. Temat niełatwy i większość uzna, że niepotrzebny. I tak to szaleństwo będzie trwać nadal :)

Opisałeś sedno problemu w punkt. Przynajmniej mnie pocieszyłeś że nie jestem jedynym który trafił na coś takiego w pracy. ;)

Ad. 2 Gdyby te nazwy metod były bardziej znaczące i tak jak piszesz metoda nie wykonywała by N rzeczy na raz to żaden taki komentarz nie byłby potrzebny. Co to jest serwis.daty() ?! Wywołałeś u mnie złe wspomnienia ;)

No daty to daty, jakieś magiczne wyciągane po regułach trochę z kodu trochę wyznaczane z procedur z bazy. To trochę takie błędne koło, piszą mutowalne metody które jednoznacznie nie dają się opisać a komentarzy do pisania w kodzie nie chcą bo autor mutowalnego potwora: dla niego jest to oczywiste i każdy ma to rozumieć. Taka sytuacja prowadzi do tego że ani to utrzymać, ani później wyjaśnić drugiej osobie i co z takim fantem robić? Jak sobie radzić? Jak żyć?

4
  1. Nie warto się kopać z koniem. Powiedz im że piszą gówniany kod i złóż wypowiedzenie i tyle. Nie warto zdrowia i nerwów. Nikt tego nie doceni a ty jeszcze dostaniesz łatkę czepialskiego. Masz ten przywilej że jako programista możesz zmienić pracę praktycznie z marszu, często na lepszych warunkach. Korzystaj z tego.
  2. To jakieś żarty? A czemu nie zrobić metodę oznaczFaktureDoRozliczenia() która robi to co pod tym komentarzem? Ewentualnie możesz dać javadoca do takiej metody/klasy. Zostawianie takich wiszących // w kodzie jest bez sensu. Za rok kod się zupełnie zmieni a to będzie wisieć.
  3. Trochę się nie zgodzę. Praktyka pokazuje że 80% wymagań nie jest znana w chwili robienia projektu ;) Poza jakimiś ekstremalnymi sytucjami, jak jakieś systemy krytyczne które potrzebują certyfikacji, nie ma sensu trzymac się designu który jest zły. Jeśli coś sie da zrobić lepiej to trzeba zwyczajnie później poprawić design. Nie mówie tu ofc o jakichś szybkich hackach! W ogóle poza wyjątkowymi sytuacjami takie waterfallowe projektowanie, że najpierw ktoś maluje diagramy całego systemu a potem ktoś to implementuje, się zwyczajnie nie sprawdza.
0
Shalom napisał(a):
  1. To jakieś żarty? A czemu nie zrobić metodę oznaczFaktureDoRozliczenia() która robi to co pod tym komentarzem? Ewentualnie możesz dać javadoca do takiej metody/klasy. Zostawianie takich wiszących // w kodzie jest bez sensu. Za rok kod się zupełnie zmieni a to będzie wisieć.

Czyli jak opisywać to na poziomie metod i klas?

  1. Trochę się nie zgodzę. Praktyka pokazuje że 80% wymagań nie jest znana w chwili robienia projektu ;) Poza jakimiś ekstremalnymi sytucjami, jak jakieś systemy krytyczne które potrzebują certyfikacji, nie ma sensu trzymac się designu który jest zły. Jeśli coś sie da zrobić lepiej to trzeba zwyczajnie później poprawić design. Nie mówie tu ofc o jakichś szybkich hackach! W ogóle poza wyjątkowymi sytuacjami takie waterfallowe projektowanie, że najpierw ktoś maluje diagramy całego systemu a potem ktoś to implementuje, się zwyczajnie nie sprawdza.

No z moich tam doświadczeń wynika że po kilku iteracjach diagram analityka jest stabilny i w miarę bezpiecznie można go trzymać zgodnego z kodem. Trochę obawiam się tego punktu: "Jeśli coś sie da zrobić lepiej to trzeba zwyczajnie później poprawić design" - właśnie widzę w robocie taki kod gdzie jest masa sprytnych "rozwiązań i optymalizacji" i nijak się ma zaimplementowany do flow opisanego w dokumentacji, w schemacie VP i weź się domyśl jak to działa. No nie domyślisz się, trzeba ślęczeć i debugować krok po kroku. Pytanie: jak unikać rozjazdów?

0
  1. Trudna sprawa, bo programiści piszący słaby kod zwykle są zadufani w sobie i traktują swój kod bardzo osobiście. Inaczej, pewnie by się edukowali i zrozumieli swój błąd. To przecież nie jest żadna nowinka. Być może jeśli byś pokazał na swoim kodzie że takie podejście jest dobre...? A jeśli nie masz prawa napisać kod jak uważasz to zmień pracę. Jak ludzie nie dają się przekonać, też. I następnym razem bardziej przygotuj się do rozmowy, spróbuj wyłapać takie problemy na etapie rozmowy kwalifikacyjnej.
  2. Przykład komentarzy który podałeś, w moim odczuciu jest nadmiarowy, bo można wyczytać to samo z kodu. Jak dla mnie komentarz to ostateczność, należy ich używać jeśli nie możesz czegoś opisać nazwami zmiennych i metod. Na przykład kiedy korzystasz z jakiegoś wzoru matematycznego i stosujesz pomijając wprowadzenie, warto je napisać w komentarzu. Podobnie z systemami zewnętrznymi, czasami trzeba napisać trochę o dziwnościach jego interfejsu.
  3. Nie wiem, nigdy nie pracowałem w zespole który stosowałby takie sztywne podejście.
1

@MrMadMatt

Czyli jak opisywać to na poziomie metod i klas?

Komentarze mają opisywać CZEMU i JAK coś się dzieje (jeśli jest niejasne) ale nie CO się dzieje. CO się dzieje ma wynikać z kodu. Więc komentarz // oznacz fakture do rozliczenia jest bez sensu, bo powinno to zwyczajnie leżeć w metodzie o takiej nazwie. Komentarz // wyszukuje z tabeli STML bla bla bla i JIRA ID można dać jako javadoc do metody/klasy, bo opisuje czemu robicie to w taki a nie inny sposób.

No z moich tam doświadczeń wynika że po kilku iteracjach diagram analityka jest stabilny i w miarę bezpiecznie można go trzymać zgodnego z kodem

To chyba tylko w systemie który się nie rozwija, albo jest jakoś mocno spięty z hardwarem jakimś. Trochę nie chce mi się wierzyć że za pół roku, rok czy dwa nie pojawią się nowe wymagania które ten cały projekt wywrócą do góry nogami.

jak unikać rozjazdów?

Nie robić diagramów.

0

Chyba jedynie przez pokazanie korzyści/możliwości, które wynikną z tego nowego pomysłu.

"2. Drugie pytanie jakie mam: Jak wygląda u was pseudokod w kodzie produkcyjnym?"

Hmm, albo masz kod produkcyjny albo pseudokod. Jeśli mam komentarze, to takie które nakreślają ogólną koncepcję i wskazuję CO kod ma robić, a nie JAK to robi.
Czasem zdarzają się wstawki w plantumlu, tak by można sobie zrobić copy&paste do narzędzia, które rozumie plantumla i pokaże diagram sekwencji ilustrujący flow.

"3. Jak wygląda wasza relacja między kodem a diagramem np. BPMN z dokumentacji analityka?"

Najczęściej* nie wygląda zupełnie, gdyż:

BMPN nie jest narzędziem do modelowania systemów, tylko procesów (...które mogą przebiegać przez różne systemy). W ramach procesu wykonywane są różne czynności. To, że jakaś czynność z procesu biznesowego jest realizowana przez system, to będzie przypadek użycia w systemie (UC) i developera powinny interesować właśnie owe przypadki użycia. Tu pomocne mogą być diagramy sekwencji dla konkretnego UC (ale to już UML, albo jakaś "notacja szyta na miarę"). Proces opisany BPMN pokaże jak ten nasz UC wpisuje się w większą całość i gdzie są konsumowane rezultaty. Pokaże też wpływ zmian naszego UC na biznes, bo jeśli nasz UC wywoływany jest w X procesach, to jak zmieniamy UC, to musimy uwzględnić wpływ na X procesów, w których uczestniczy Y systemów.

    • specyficznym przypadkiem jest korzystanie z silnika procesów biznesowych, ale wówczas procesy opisywane są na zupełnie innym poziomie szczegółowości. Można też zrobić własny silniczek procesów i mówić, że realizuje sagi ;)
0
yarel napisał(a):

Najczęściej* nie wygląda zupełnie, gdyż:

BMPN nie jest narzędziem do modelowania systemów, tylko procesów (...które mogą przebiegać przez różne systemy). W ramach procesu wykonywane są różne czynności. To, że jakaś czynność z procesu biznesowego jest realizowana przez system, to będzie przypadek użycia w systemie (UC) i developera powinny interesować właśnie owe przypadki użycia. Tu pomocne mogą być diagramy sekwencji dla konkretnego UC (ale to już UML, albo jakaś "notacja szyta na miarę"). Proces opisany BPMN pokaże jak ten nasz UC wpisuje się w większą całość i gdzie są konsumowane rezultaty. Pokaże też wpływ zmian naszego UC na biznes, bo jeśli nasz UC wywoływany jest w X procesach, to jak zmieniamy UC, to musimy uwzględnić wpływ na X procesów, w których uczestniczy Y systemów.

    • specyficznym przypadkiem jest korzystanie z silnika procesów biznesowych, ale wówczas procesy opisywane są na zupełnie innym poziomie szczegółowości. Można też zrobić własny silniczek procesów i mówić, że realizuje sagi ;)

Z tym BPMN'em się machnąłem bo faktycznie miałem na myśli schemat blokowy, w sensie schemat z bloczków gdzie masz opisane działanie jakiejś metody i wskazówki jak dany krok w metodzie wykonać technicznie na bazie np. jest bloczek o nazwie 'zweryfikuj fakturę' a w szczegółach opis jak zrobić to na bazie np. "sprawdź czy w tabeli ABC jest rekord powiązany po kolumnie f_id bla bla bla". Mea culpa. I interesuje mnie jak blisko trzymać kod przy takim schemacie. Z jednej strony schemat się zmienia i kod w którymś momencie się rozjedzie z dokumentacją a znowu widziałem już kod który nijak się ma do schematu, "bo optymalizacje" i w kodzie pojedynczego kroku/bloczka ze schematu nie znajdziesz, nie zmodyfikujesz bo kod odleciał. Interesuje mnie "jak to robi konkurencja". ;)

7
  1. nie przekonasz, rob po swojemu i tez zmieniaj kod innych na dobry, ale upewnij sie ze rozumiesz ich zdanie i to nie ty jestes glabem
  2. poza easter-eggami komentarze maja zerowa wartosc
  3. analitycy sa generalnie bezuzyteczni poza bardzo specyficznymi wyjatkami. to raczej proste - gdyby byli dobrymi koderami to by kodowali a nie "analizowali" tak wiec ich wytyczne dotyczace kodu/architektury sa w wiekszosci przypadkow bezwartosciowe dla ogarnietego programisty.
0
katelx napisał(a):
  1. nie przekonasz, rob po swojemu i tez zmieniaj kod innych na dobry, ale upewnij sie ze rozumiesz ich zdanie i to nie ty jestes glabem
  2. poza easter-eggami komentarze maja zerowa wartosc
  3. analitycy sa generalnie bezuzyteczni poza bardzo specyficznymi wyjatkami. to raczej proste - gdyby byli dobrymi koderami to by kodowali a nie "analizowali" tak wiec ich wytyczne dotyczace kodu/architektury sa w wiekszosci przypadkow bezwartosciowe dla ogarnietego programisty.
  1. Niby fajnie ale jak klepnę coś po swojemu to jest krzyk na code review i rejected tak długo aż nie sprowadzę tego do poziomu akceptowalnego przez nich, czytaj: sprytnie, mało i z jak największą ilością ilością ficzerów/sprytnej składni języka.
  2. Mi się zdarzyło znaleźć i zostawić w kodzie komentarz który jak się później okazało, był pomocny. Taki smaczek "ktoś tu przed nami był, to byli ludzie" - coś jak malunki mamutów w opuszczonych jaskiniach. ;)
  3. Czyyy ja wiem, nie mam takiego zdania o analitykach których spotkałem. Może to przez to że spora część z nich była wcześniej koderami tylko przeszli na drugą stronę. Ciężko mi się z Twoim stwierdzeniem zgodzić jak i ja mogę nie mieć dość obycia. Trudno powiedzieć.
2

Przekonywanie członków zespołu do nowych pomysłów - jak to w PL = łapówki, prezenty, przysługi itp. :)

2
MrMadMatt napisał(a):

Hej,
Jeżeli temat nie pasuje do działu to proszę o przeniesienie, mam do was następujące pytania:

  1. Jak przekonać zespół, w którym się pracuje do tego że mutowalność jest zła? Chodzi mi o to że prywatnie mam już za sobą etap w życiu gdy pisałem voidową metodę z 5 parametrami, gdzie dwa z nich były modyfikowane a jeden był switchem i wiem że katastrofalnie się to kończyło gdy po paru miesiącach trzeba było „doklepać mały ficzer” w takim miejscu. W obecnej pracy praktyka ta wydaje się być dość powszechna bo można „dwie pieczenie na jednym ogniu i przy okazji, oszczędność w kodzie, bo im mniej kodu tym lepiej”. Jakie argumenty można przedstawić, prace, artykuły, dowody? Nie mam dość autorytetu ze względu na staż pracy w obecnym miejscu przez co powołanie się na kogoś „z góry” byłoby ok. Ten sam problem tyczy się obiektów DTO gdzie w swoim cyklu życia zmienia ono stan kilka razy poprzez setery.

Najprościej zmienić pracę na taką, w której będą pracować bardziej doświadczeni programiści. Bo widocznie twoi współpracownicy to juniorzy z wysokim ego i z władzą w projekcie. Nic nie poradzisz. Szczególnie jeśli ktoś ma wysiedziane dupogodziny i wydaje mu się, że wszystko wie, a nie zdaje sobie sprawy z tego, że jest słaby i że przez lata tylko budował złe nawyki.

Ale z drugiej strony myślę, że w drugą stronę też można przesadzić i wciskać niemutowalnosć tam, gdzie jest nie potrzebna, a tylko pogarsza wydajność czy komplikuje kod (szczególnie jeśli dany język programowania nie wspiera jej out of the box). Fanatycy programowania funkcyjnego też istnieją.

0
LukeJL napisał(a):

Najprościej zmienić pracę na taką, w której będą pracować bardziej doświadczeni programiści. Bo widocznie twoi współpracownicy to juniorzy z wysokim ego i z władzą w projekcie. Nic nie poradzisz. Szczególnie jeśli ktoś ma wysiedziane dupogodziny i wydaje mu się, że wszystko wie, a nie zdaje sobie sprawy z tego, że jest słaby i że przez lata tylko budował złe nawyki.

Ale z drugiej strony myślę, że w drugą stronę też można przesadzić i wciskać niemutowalnosć tam, gdzie jest nie potrzebna, a tylko pogarsza wydajność czy komplikuje kod (szczególnie jeśli dany język programowania nie wspiera jej out of the box). Fanatycy programowania funkcyjnego też istnieją.

Czyli takie zjawisko nie jest czymś wyjątkowym (juniorzy z ego z władzą w projekcie) i naturalnie występuje w przyrodzie? Odpisałem w jednym z postów że oczywiście zdaje sobie sprawę z tego że trzeba brać zdanie innych aby nie wyjść na głąba/jełopa i krytycznie podchodzić nawet do swoich pomysłów i weryfikować czy mam racje. Problem jaki mam to to że mój kod jest prasowany do standardów firmy gdzie owe standardy są.. meh.

0

a tylko pogarsza wydajność

Tyle że rzadko się zdarza żeby to była duża różnica. Jeżeli masz typową biznesowa aplikacje backendowa to ona "muli" na IO (bazach danych szczególnie).
Generalnie z mojego doświadczenia wynika że 80% klas w enterprajsowej Javie może być niemutowalna ( i powinna być).

1

ad .1 Drążyć, robić swoje.
Zmienianie zespołu i firmy to często pułapka - trafa w innej firmie zawsze zielona.
Ale jest to opcja. U mnie papierek lakmusowy jest taki -> jeśli goście twierdzą, że mają w kodzie syf i powoli to sprzątają - to warto rozmawiać. Prawdziwy syf to mają właśnie Ci co są ultra zadowoleni.
Co do mutowalności - warto pokazywać ile błędów przez nią powstaje (ja to robiłem, jakoś działało, chociaż nie spektakularnie).

ad 2. Co do komentarzy mam zdanie
w tym kodzie jedynie ten : // nikt nic nie wie, wszyscy co to pisali się zwolnili, szukaj w JIRA-0101 ma jakiś sens.
Reszta do wywalenia i pozmieniać kod.

ad 3. Trzeba ocenić samemu na ile diagramy są sensowne. Robiłem opcje blisko diagramów, nawet jak bolało, kiedy widziałem, że to rzeczywście nasz biznes.
Diagramy wysokopoziomowe, wydumane itp. olewam. (Ciekawostka: mamy w fimie też system, który odpala wprost BPMNowe twory - trochę straszne, trochę ciekawe, praca blisko engine BPMN jest nawet intereująca (dla programisty), ale praca z diagramami to koszmar).

0

Ad 2: pseudokod w kodzie.
Czyli wysokopoziomowe wywolania o nazwach bardziej znaczacych niz ZXVRL12 (przyklad z zycia).
Komentarze sa dobre do publicznego API (javadoc, openapi) po to zeby nie trzeba bylo zagladac do srodka.
Jak juz jestes w kodzie to powinien byc na tyle przejrzysty zeby dalo sie go przeczytac bez analizy kodu wywolywanego i na tyle krotki zeby moc go zapamietac.

Jesli masz potrzebe rozdzielenia sekcji kodu komentarzami to prawdopodobnie robisz wiecej niz jedna rzecz w funkcji / metodzie.

2

Próbowałem przekonywać zespół do dobrych praktyk ze 3 razy. Niestety zawsze niepowodzenie, już się nasłuchałem wymówek chyba wszystkich możliwych.

  1. Biznes nie pozwoli na lepsze pisanie kodu bo nie widzi w tym wartości. Ważne by było szybko i działało.
  2. Nie można poprawiać kodu bo testerzy nie przetestują.
  3. Zaczniemy pisać testy od następnego sprintu/projektu/roku
  4. Dobrze, możemy zacząć wprowadzać dobre praktyki, ale najpierw potrzebujemy posiedzenia wszystkich programistów, a na nie nie ma czasu.
  5. Wcale nie jest tak źle, w sumie to lubimy tak pisać
  6. Nie chcemy by ktoś nas oceniał na Code Review, przecież dobrze piszemy kod

Przez chwilę można próbować, ale zmień firmę jak czujesz, że to już bicie głową o ścianę.

5

Nie wiem w czym problem, od tego jest review zeby głupie rozwiązania nie przechodziły dalej. To samo tyczy się braku testów itp. I jeszcze to zwalanie że "biznes nie pozwoli na lepsze pisanie kodu". Od kiedy sie tłumaczycie z pisania kodu biznesowi? Biznes nie wie nic o wytwarzaniu kodu, od tego zatrudnia was. Etymujecie zadanie na tyle że ma być dowiezione poprawnie zgodnie ze sztuką. Testy to nie jest jakaś wydzielona poboczna część wytwarzania oprogramowania, i klient nie decyduje o jej opcjonalnośći. No ale swoje lenistwo i niekompetencje najlepiej zwalić na biznes. "Byśmy chcieli pisać lepszy kod ale klient taki i siaki", [CIACH!] programistycznego planktonu IT.

1
cmd napisał(a):

Nie wiem w czym problem, od tego jest review zeby głupie rozwiązania nie przechodziły dalej. To samo tyczy się braku testów itp.

Problem w tym, że nie wszędzie jest kultura robienia review czy pisania testów. A jak jej nie ma, to siłą się jej nie wprowadzi. Przecież to nie biznes ma rozumieć, że review czy testy są potrzebne, to programiści muszą.

0

Jeśli chodzi o komentarze kodu to 99% przypadkach nie powinny występować, jeśli piszesz dobry kod, to dokumentuje się sam w sobie. W ostateczności można dodać javaDoc nad metoda najlepiej w interfejsie, jeśli faktycznie robi coś skomplikowanego i nieoczywistego.
Więc jeśli chciałbyś wskazać autorytety którzy to rekomendują, to nie będzie to trudne, wystarczy powołać się na uncle boba.

Co do stosowania fitcherow językowych, to mam nadzieję że nie chodziło o stosowanie Javy 8+
Wciąż często spotykam programistów którzy twierdzą że dla nich jest to nieczytelne, jednak to wynika tylko i wyłącznie z faktu, że nie chce im się nauczyć nowych rzeczy. Żaden programista który nauczył się programowania funkcyjnego nie powie, że pętla for z zapetlonymi ifami będzie mniej czytelną.
Oczywiście nie powinniśmy stosować tego zawsze i wszędzie. Czasem lepiej będzie użyć zwykłej pętli for.
W razie jakby zespół z którym chcesz współpracować nie chciał się dostosowywać do tego, to możesz polecić np. książkę effective java. Tam Joshua Bosh bardzo dobrze tłumaczy jak i kiedy stosować streamy i optionale.

Ze znalezieniem autorytetu który mówi o finalnych klasach dto z finalnymi polami i bez setterow raczej nie powinno być problemów, to jest podstawa.

Swoją drogą też kiedyś spotykałem osoby które nie chciały się przekonać do podejścia bardziej obiektowego, wydzielania metody na pomniejsze klasy, w efekcie czego powstawały kwiatki z metodą na 300 linii kodu z milionami forów ifow, ale taka rada, jakbyś czasem natrafił na taki kod, to po prostu powiedz koledze żeby go przetestował, sam zobaczysz że zacznie go refaktoryzowac bez żadnego namawiana do tego, bo testu nie będzie w stanie napisać. Chyba że w ogóle nie piszecie testów, to wtedy to już kaplica niestety, a wiem że takie przypadki też się zdarzaja nawet w tych czasach.

0

Skąd ta silna awersja do komentarzy? Czy wynika to z błędnej interpretacji jednego z "autorytetów" czy może braku kontaktu z kodem typu

float Q_rsqrt( float number )
{
	long i;
	float x2, y;
	const float threehalfs = 1.5F;

	x2 = number * 0.5F;
	y  = number;
	i  = * ( long * ) &y;                       // evil floating point bit level hacking
	i  = 0x5f3759df - ( i >> 1 );               // what the fuck? 
	y  = * ( float * ) &i;
	y  = y * ( threehalfs - ( x2 * y * y ) );   // 1st iteration
//	y  = y * ( threehalfs - ( x2 * y * y ) );   // 2nd iteration, this can be removed

	return y;
}

zresztą nawet w mega kródach jest wiele okazji do dorzucenia opisu dla jakichś edge casów biznesu, więc skąd ta niechęć?

bo komentarze się deaktualizują? kod też się deaktualizuje i trzeba go zaktualizować, więc komentarze też można.

1

Na prawdę ?
W moim poście chodziło o to, żeby pisać kod taki aby ten komentarz nie był potrzebny...ten kod nigdy nie powinien przejść CR.
Pisać nazwę zmiennej 'y' tylko po to żeby mieć powód napisać komentarz ?
Dlaczego nie nazwać odpowiednio tej metody oraz zmiennych ? Wydzielić do mniejszych metod prywatnych które nam powiedzą co tam się dzieje ?

Co masz na myśli przez błędna interpretację, możesz doprecyzować ?

0

@AlfProgrammer:

Ja piszę o tych skrajnościach typu - comments are code smell

Co masz na myśli przez błędna interpretację, możesz doprecyzować ?

Niejednokrotnie spotkałem się z taką interpretacją w której wujo twierdzi, że komentarze = zło.

Dlaczego nie nazwać odpowiednio tej metody oraz zmiennych ? Wydzielić do mniejszych metod prywatnych które nam powiedzą co tam się dzieje ?

Jasne, można rozbić 5 linijek na 5 funkcji, nadać im zgrabną, przejrzystą nazwę i uczynić debugowanie tego męczarnią, a bonusowe punkty będą jeżeli każda z tych metod będzie w osobnym pliku. 20+ znakowe nazwy zmiennych też nie są fajne :D

albo można też napisać komentarze :D

1

Popadamy od skrajności w skrajność. Nikt nie mówi, że WSZYSTKIE KOMENTARZE są złe. Chodzi o sytuacje, gdzie metoda nazywa się data zamiast nazwać ją tak, że człowiek od razu wie czy ta metoda tą datę pobiera, zapisuje czy cokolwiek. Rozumiem, że komentarz w metodzie, która ma 150 linijek i zawiera zmienne x będzie łatwiej debugować? (tak, to też skrajność)

0

Generalnie nie zawsze da się zrobić w pełni tak żeby same nazwy (nie)zmiennych i funkcji wszystko wyjasniały, ale komentarze rzeczywiście powinny być używane rzadko - własnie w takich sytuacjach żeby opisac cos bardziej złozonego.

1

Komentarze powinny być używane tak często, jak trzeba.

W crudach będzie tego znacznie mniej, ale w algorytmach może być dużo.

Nie będąc gołosłownym podrzucam krótki kod, który ludzie na HNie określili jako

Other than that, this is incredibly clean code and I doubt you can get much more performance without dropping into assembly. I wish my coworkers made code this well documented :/

A komentarzy jest w nim mnóstwo (szczególnie podoba mi się 260 linijka).

https://github.com/zwegner/faster-utf8-validator/blob/master/z_validate.c

// faster-utf8-validator
//
// Copyright (c) 2019 Zach Wegner
// 
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
// in the Software without restriction, including without limitation the rights
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
// copies of the Software, and to permit persons to whom the Software is
// furnished to do so, subject to the following conditions:
// 
// The above copyright notice and this permission notice shall be included in
// all copies or substantial portions of the Software.
// 
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
// SOFTWARE.

#include <stdint.h>
#include <immintrin.h>

// How this validator works:
//
//   [[[ UTF-8 refresher: UTF-8 encodes text in sequences of "code points",
//   each one from 1-4 bytes. For each code point that is longer than one byte,
//   the code point begins with a unique prefix that specifies how many bytes
//   follow. All bytes in the code point after this first have a continuation
//   marker. All code points in UTF-8 will thus look like one of the following
//   binary sequences, with x meaning "don't care":
//      1 byte:  0xxxxxxx
//      2 bytes: 110xxxxx  10xxxxxx
//      3 bytes: 1110xxxx  10xxxxxx  10xxxxxx
//      4 bytes: 11110xxx  10xxxxxx  10xxxxxx  10xxxxxx
//   ]]]
//
// This validator works in two basic steps: checking continuation bytes, and
// handling special cases. Each step works on one vector's worth of input
// bytes at a time.
//
// The continuation bytes are handled in a fairly straightforward manner in
// the scalar domain. A mask is created from the input byte vector for each
// of the highest four bits of every byte. The first mask allows us to quickly
// skip pure ASCII input vectors, which have no bits set. The first and
// (inverted) second masks together give us every continuation byte (10xxxxxx).
// The other masks are used to find prefixes of multi-byte code points (110,
// 1110, 11110). For these, we keep a "required continuation" mask, by shifting
// these masks 1, 2, and 3 bits respectively forward in the byte stream. That
// is, we take a mask of all bytes that start with 11, and shift it left one
// bit forward to get the mask of all the first continuation bytes, then do the
// same for the second and third continuation bytes. Here's an example input
// sequence along with the corresponding masks:
//
//   bytes:        61 C3 80 62 E0 A0 80 63 F0 90 80 80 00
//   code points:  61|C3 80|62|E0 A0 80|63|F0 90 80 80|00
//   # of bytes:   1 |2  - |1 |3  -  - |1 |4  -  -  - |1
//   cont. mask 1: -  -  1  -  -  1  -  -  -  1  -  -  -
//   cont. mask 2: -  -  -  -  -  -  1  -  -  -  1  -  -
//   cont. mask 3: -  -  -  -  -  -  -  -  -  -  -  1  -
//   cont. mask *: 0  0  1  0  0  1  1  0  0  1  1  1  0
//
// The final required continuation mask is then compared with the mask of
// actual continuation bytes, and must match exactly in valid UTF-8. The only
// complication in this step is that the shifted masks can cross vector
// boundaries, so we need to keep a "carry" mask of the bits that were shifted
// past the boundary in the last loop iteration.
//
// Besides the basic prefix coding of UTF-8, there are several invalid byte
// sequences that need special handling. These are due to three factors:
// code points that could be described in fewer bytes, code points that are
// part of a surrogate pair (which are only valid in UTF-16), and code points
// that are past the highest valid code point U+10FFFF.
//
// All of the invalid sequences can be detected by independently observing
// the first three nibbles of each code point. Since AVX2 can do a 4-bit/16-byte
// lookup in parallel for all 32 bytes in a vector, we can create bit masks
// for all of these error conditions, look up the bit masks for the three
// nibbles for all input bytes, and AND them together to get a final error mask,
// that must be all zero for valid UTF-8. This is somewhat complicated by
// needing to shift the error masks from the first and second nibbles forward in
// the byte stream to line up with the third nibble.
//
// We have these possible values for valid UTF-8 sequences, broken down
// by the first three nibbles:
//
//   1st   2nd   3rd   comment
//   0..7  0..F        ASCII
//   8..B  0..F        continuation bytes
//   C     2..F  8..B  C0 xx and C1 xx can be encoded in 1 byte
//   D     0..F  8..B  D0..DF are valid with a continuation byte
//   E     0     A..B  E0 8x and E0 9x can be encoded with 2 bytes
//         1..C  8..B  E1..EC are valid with continuation bytes
//         D     8..9  ED Ax and ED Bx correspond to surrogate pairs
//         E..F  8..B  EE..EF are valid with continuation bytes
//   F     0..3  8..B  F0..F3 are valid with continuation bytes
//         4     8     F4 8F BF BF is the maximum valid code point
//
// That leaves us with these invalid sequences, which would otherwise fit
// into UTF-8's prefix encoding. Each of these invalid sequences needs to
// be detected separately, with their own bits in the error mask.
//
//   1st   2nd   3rd   error bit
//   C     0..1  0..F  0x01
//   E     0     8..9  0x02
//         D     A..B  0x04
//   F     4     9..F  0x08
//   F     5..F  0..F  0x10
//
// For every possible value of the first, second, and third nibbles, we keep
// a lookup table that contains the bitwise OR of all errors that that nibble
// value can cause. For example, the first nibble has zeroes in every entry
// except for C, E, and F, and the third nibble lookup has the 0x11 bits in
// every entry, since those errors don't depend on the third nibble. After
// doing a parallel lookup of the first/second/third nibble values for all
// bytes, we AND them together. Only when all three have an error bit in common
// do we fail validation.

#if defined(AVX2)

// AVX2 definitions

#   define z_validate_utf8  z_validate_utf8_avx2
#   define z_validate_vec   z_validate_vec_avx2

#   define V_LEN            (32)

// Vector and vector mask types. We use #defines instead of typedefs so this
// header can be included multiple times with different configurations

#   define vec_t            __m256i
#   define vmask_t          uint32_t
#   define vmask2_t         uint64_t

#   define v_load(x)        _mm256_loadu_si256((vec_t *)(x))
#   define v_set1           _mm256_set1_epi8
#   define v_movemask       _mm256_movemask_epi8
#   define v_shift_left     _mm256_slli_epi16
#   define v_shift_right    _mm256_srli_epi16
#   define v_and            _mm256_and_si256
#   define v_shuffle        _mm256_shuffle_epi8
#   define v_testz          _mm256_testz_si256

// Simple macro to make a vector lookup table for use with vpshufb. Since
// AVX2 is two 16-byte halves, we duplicate the input values.

#   define V_LOOKUP16(...)    _mm256_setr_epi8(__VA_ARGS__, __VA_ARGS__)

#   define v_shift_lanes_left v_shift_lanes_left_avx2

// Move all the bytes in "top" to the left by one and fill in the first byte
// with the last byte in "bottom". Since AVX2 generally works on two separate
// 16-byte vectors glued together, this needs two steps. The permute2x128 takes
// the middle 32 bytes of the 64-byte concatenation bottom:top. The align then
// gives the final result in each half:
//      top half:    top_L:top_H -->    top_L[15]:top_H[0:14]
//   bottom half: bottom_H:top_L --> bottom_H[15]:top_L[0:14]
static inline vec_t v_shift_lanes_left(vec_t top, vec_t bottom) {
    vec_t shl_16 = _mm256_permute2x128_si256(top, bottom, 0x03);
    return _mm256_alignr_epi8(top, shl_16, 15);
}

#elif defined(SSE4)

// SSE definitions. We require at least SSE4.1 for _mm_test_all_zeros()

#   define z_validate_utf8  z_validate_utf8_sse4
#   define z_validate_vec   z_validate_vec_sse4

#   define V_LEN            (16)

#   define vec_t            __m128i
#   define vmask_t          uint16_t
#   define vmask2_t         uint32_t

#   define v_load(x)        _mm_lddqu_si128((vec_t *)(x))
#   define v_set1           _mm_set1_epi8
#   define v_movemask       _mm_movemask_epi8
#   define v_shift_left     _mm_slli_epi16
#   define v_shift_right    _mm_srli_epi16
#   define v_and            _mm_and_si128
#   define v_shuffle        _mm_shuffle_epi8
#   define v_testz          _mm_test_all_zeros

#   define V_LOOKUP16(...)  _mm_setr_epi8(__VA_ARGS__)

#   define v_shift_lanes_left v_shift_lanes_left_sse4
static inline vec_t v_shift_lanes_left(vec_t top, vec_t bottom) {
    return _mm_alignr_epi8(top, bottom, 15);
}

#else

#   error "No valid configuration: must define one of AVX2 or SSE4

#endif

// Validate one vector's worth of input bytes
inline int z_validate_vec(vec_t bytes, vec_t shifted_bytes, vmask_t *last_cont) {
    // Error lookup tables for the first, second, and third nibbles
    const vec_t error_1 = V_LOOKUP16(
        0x00, 0x00, 0x00, 0x00,
        0x00, 0x00, 0x00, 0x00,
        0x00, 0x00, 0x00, 0x00,
        0x01, 0x00, 0x06, 0x18
    );
    const vec_t error_2 = V_LOOKUP16(
        0x03, 0x01, 0x00, 0x00,
        0x08, 0x10, 0x10, 0x10,
        0x10, 0x10, 0x10, 0x10,
        0x10, 0x14, 0x10, 0x10
    );
    const vec_t error_3 = V_LOOKUP16(
        0x11, 0x11, 0x11, 0x11,
        0x11, 0x11, 0x11, 0x11,
        0x13, 0x1B, 0x1D, 0x1D,
        0x19, 0x19, 0x19, 0x19
    );

    // Mask for table lookup indices
    const vec_t mask_0F = v_set1(0x0F);

    // Quick skip for ascii-only input
    vmask_t high = v_movemask(bytes);
    if (!(high | *last_cont))
        return 1;

    // Which bytes are required to be continuation bytes
    vmask2_t req = *last_cont;
    // A bitmask of the actual continuation bytes in the input
    vmask_t cont;

    // Compute the continuation byte mask by finding bytes that start with
    // 11x, 111x, and 1111. For each of these prefixes, we get a bitmask
    // and shift it forward by 1, 2, or 3. This loop should be unrolled by
    // the compiler, and the (n == 1) branch inside eliminated.
    vmask_t set = high;
    for (int n = 1; n <= 3; n++) {
        vec_t bit = v_shift_left(bytes, n);
        set &= v_movemask(bit);
        // Mark continuation bytes: those that have the high bit set but
        // not the next one
        if (n == 1)
            cont = high ^ set;

        // We add the shifted mask here instead of ORing it, which would
        // be the more natural operation, so that this line can be done
        // with one lea. While adding could give a different result due
        // to carries, this will only happen for invalid UTF-8 sequences,
        // and in a way that won't cause it to pass validation. Reasoning:
        // Any bits for required continuation bytes come after the bits
        // for their leader bytes, and are all contiguous. For a carry to
        // happen, two of these bit sequences would have to overlap. If
        // this is the case, there is a leader byte before the second set
        // of required continuation bytes (and thus before the bit that
        // will be cleared by a carry). This leader byte will not be
        // in the continuation mask, despite being required. QEDish.
        req += (vmask2_t)set << n;
    }
    // Check that continuation bytes match. We must cast req from vmask2_t
    // (which holds the carry mask in the upper half) to vmask_t, which
    // zeroes out the upper bits
    if (cont != (vmask_t)req)
        return 0;

    // Look up error masks for three consecutive nibbles. We need to
    // AND with 0x0F for each one, because vpshufb has the neat
    // "feature" that negative values in an index byte will result in 
    // a zero.
    vec_t m_1 = v_and(v_shift_right(shifted_bytes, 4), mask_0F);
    vec_t e_1 = v_shuffle(error_1, m_1);

    vec_t m_2 = v_and(shifted_bytes, mask_0F);
    vec_t e_2 = v_shuffle(error_2, m_2);

    vec_t m_3 = v_and(v_shift_right(bytes, 4), mask_0F);
    vec_t e_3 = v_shuffle(error_3, m_3);

    // Check if any bits are set in all three error masks
    if (!v_testz(v_and(e_1, e_2), e_3))
        return 0;

    // Save continuation bits and input bytes for the next round
    *last_cont = req >> V_LEN;
    return 1;
}

int z_validate_utf8(const char *data, size_t len) {
    vec_t bytes, shifted_bytes;

    // Keep continuation bits from the previous iteration that carry over to
    // each input chunk vector
    vmask_t last_cont = 0;

    size_t offset = 0;
    // Deal with the input up until the last section of bytes
    if (len >= V_LEN) {
        // We need a vector of the input byte stream shifted forward one byte.
        // Since we don't want to read the memory before the data pointer
        // (which might not even be mapped), for the first chunk of input just
        // use vector instructions.
        shifted_bytes = v_shift_lanes_left(v_load(data), v_set1(0));

        // Loop over input in V_LEN-byte chunks, as long as we can safely read
        // that far into memory
        for (; offset + V_LEN < len; offset += V_LEN) {
            bytes = v_load(data + offset);
            if (!z_validate_vec(bytes, shifted_bytes, &last_cont))
                return 0;
            shifted_bytes = v_load(data + offset + V_LEN - 1);
        }
    }
    // Deal with any bytes remaining. Rather than making a separate scalar path,
    // just fill in a buffer, reading bytes only up to len, and load from that.
    if (offset < len) {
        char buffer[V_LEN + 1] = { 0 };
        if (offset > 0)
            buffer[0] = data[offset - 1];
        for (int i = 0; i < (int)(len - offset); i++)
            buffer[i + 1] = data[offset + i];

        bytes = v_load(buffer + 1);
        shifted_bytes = v_load(buffer);
        if (!z_validate_vec(bytes, shifted_bytes, &last_cont))
            return 0;
    }

    // The input is valid if we don't have any more expected continuation bytes
    return last_cont == 0;
}

// Undefine all macros

#undef z_validate_utf8
#undef z_validate_vec
#undef V_LEN
#undef vec_t
#undef vmask_t
#undef vmask2_t
#undef v_load
#undef v_set1
#undef v_movemask
#undef v_shift_left
#undef v_shift_right
#undef v_and
#undef v_shuffle
#undef v_testz
#undef V_LOOKUP16
#undef v_shift_lanes_left

1

Dobry przykład. Opis algorytmu nawet spoko, ale te całe akcje pod spodem to naprawdę potrzebne:

 // Loop over input in V_LEN-byte chunks, as long as we can safely read
 // that far into memory
 for (; offset + V_LEN < len; offset += V_LEN) {
         [...]
        }

Serio looop over input in chunks ?? No, nigdy bym się nie domyślił.

To piękne:

// Which bytes are required to be continuation bytes
    vmask2_t req = *last_cont;

Clean code pełną gębą. Nazwijmy krótko zwięźle zmienną i za to wyjaśnijmy co znaczy w komentarzu - każdy będzie zadowolony.

Tutaj - brak konsekwencji:

 // Save continuation bits and input bytes for the next round
    *last_cont = req >> V_LEN;
    return 1;

A gdzie //returns two przed linijką z return 1? Brak komentarza, i skąd ja mam wiedzieć co ta ostatnia linijka teraz robi....

1 użytkowników online, w tym zalogowanych: 0, gości: 1