Jak odrzucić recenzję kodu, która Twoim zdaniem jest niepotrzebna?

89

Jestem w sytuacji, w której poproszono mnie o przejrzenie kodu, który rozwiązuje problem, który według mnie nie istnieje.

Fixer, który jest starszy ode mnie, twierdzi, że jego poprawka jest konieczna, ale wydaje mi się, że nie jest to nic innego jak zaawansowanie w C ++. Częścią naszego procesu wdrażania jest przegląd kodu, a jako drugi najwyższy inżynier w małej firmie, mam obowiązek przeglądu zmian.

Uważam, że recenzenci są tak samo odpowiedzialni za zmiany kodu, jak oryginalny programista, i nie chcę ponosić odpowiedzialności za tę zmianę. Jak poszedłbyś na temat odrzucenia tej recenzji?

James
źródło
113
Wydaje mi się to oczywiste. Ci podkreślić w przeglądzie , że kod jest w C ++ masturbacji intelektualnej, której celem jest rozwiązuje problem, który nie istnieje. A następnie, w ramach procesu recenzji, odrzucasz kod.
user16764 24.04.13
86
Nie używaj słów „masturbacja intelektualna”, gdy ją odrzucasz. Jeśli użyjesz tego sformułowania, będziesz go antagonizować, a on nie zobaczy tego, co masz do powiedzenia, jako potencjalnie poprawną krytykę techniczną, zobaczy to jako osobisty atak. Może to być dokładnie intelektualna masturbacja, ale możesz być absolutnie pewien, że on nie widzi tego w ten sposób.
Michael Shaw,
15
James - usunąłem emocje z twojego pytania, aby umożliwić społeczności skupienie się na twoim podstawowym pytaniu. Jak podkreślają inni, użycie załadowanej terminologii w twojej recenzji tylko pogorszy to, co jest wyraźnie delikatną sytuacją.
8
C i C ++ są pełne rzeczy, które wydają się działać, ale w rzeczywistości są niezdefiniowanym zachowaniem. UB to prawdziwa wada, nawet jeśli nie możesz napisać testu, który pokazuje, że nie działa tak, jak powinien. Na przykład wiele kompilatorów wykorzystuje UB jako okazję do optymalizacji, zakładając, że niezdefiniowany przypadek nigdy się nie zdarza. Na przykład, jeśli size > size+1sprawdziłeś przepełnienie liczby całkowitej ze znakiem, kompilator może po prostu zastąpić to wyrażenie false, ponieważ przepełnienie liczb całkowitych ze znakiem jest niezdefiniowane. Zobacz blog.llvm.org/2011/05/…
CodesInChaos,
5
@MichaelShaw „zobaczy to jako osobisty atak”. co brzmi dla mnie jak pytanie: osobisty atak na starszego programistę, z którym OP ma odmienną opinię, może teraz „wygrać”, ponieważ otrzymał władzę.
jwenting

Odpowiedzi:

274

Zapytaj o przypadek testowy, który zakończy się niepowodzeniem bez zmiany, która się powiedzie.

Jeśli nie jest w stanie go wyprodukować, wykorzystasz to jako uzasadnienie.

Jeśli może go wyprodukować, musisz wyjaśnić, dlaczego test jest nieważny.

Craig
źródło
245
A może, jeśli uda mu się go wyprodukować, ponownie przeanalizujesz swoje założenia i pomyślisz, że mógł mieć rację. Przypuszczalnie celem tego ćwiczenia jest poprawa kodu, a nie wygrywanie debaty.
Caleb
99
To, że nie możesz napisać testu, nie oznacza, że ​​nie jest zepsuty. Niezdefiniowane zachowanie co zazwyczaj dzieje się działać zgodnie z oczekiwaniami (C i C ++, które są pełne), warunki wyścigu potencjalną zmianę kolejności ze względu na słaby model pamięci ...
CodesInChaos
29
A co ze standardowym refaktoryzacją? Jak napisać test negatywny dla fragmentu refaktoryzacji?
Mike Weller,
62
„Poproś go, aby udowodnił, dlaczego jest to konieczne ...” Może poproś go, aby nauczył cię, dlaczego jest to konieczne.
Mike Sherrill „Cat Recall”
8
@RhysW: Błędne założenie. Istniejącego kodu z warunkami wyścigu również nie można przetestować pod tym względem. Nowy kod jest więc teoretycznie lepszy i empirycznie równoważny. Twoje założenie jest skuteczne tylko wtedy, gdy stary kod jest empirycznie lepszy.
MSalters
152

Recenzenci powinni być obiektywni.

Oczywiste jest, że sformułowałeś opinię na temat omawianego kodu, zanim jeszcze go przejrzałeś, i wygląda na to, że ty i osoba ustalająca ustaliły pozycje. Jeśli tak, to będziesz miał trudny czas do osiągnięcia celu, a jeszcze trudniejszy będzie obiektywny. Nic z tego nie pomaga procesowi i być może najlepszą i najbardziej obiektywną rzeczą, jaką możesz zrobić, jest ukłonić się, ponieważ jesteś zbyt blisko problemu.

Rozważ podejście zespołowe.

Jeśli nie możesz usunąć siebie, być może możesz poprosić kilku innych inżynierów o sprawdzenie kodu w tym samym czasie. Albo zgodzą się z tobą, że kod powinien zostać odrzucony, albo nie. Jeśli się z tobą zgodzą, to nie będzie już tylko ty kontra mocarz, a będziesz w stanie uzasadnić, że zespół spojrzał na poprawkę obiektywnie i postanowił nie zaakceptować. Z drugiej strony, jeśli zdecydują się zaakceptować poprawkę, będzie to również decyzja zespołu. Powinno być oczywiste, że powinieneś uczestniczyć z tak otwartym umysłem, jak potrafisz, i że nie powinieneś próbować wpływać na opinie innych członków zespołu w sposób inny niż racjonalna dyskusja. Ważne: jeśli później wynik będzie zły, nie wrzucaj zespołu pod autobus, mówiąc „Cóż, ja zawsze mówiłem, że to zły kod, ale inni członkowie zespołu mieli przewagę liczebną ”.

Odrzucenia są naturalną częścią procesu przeglądu kodu.

Proces przeglądu kodu nie obejmuje poprawek pieczątek od starszych osób; służy do ochrony i poprawy jakości kodu. Nie ma nic złego w odrzuceniu poprawki, pod warunkiem, że zrobisz to z właściwego powodu, tj. Że poprawka nie poprawia kodu. Jeśli po otwartej analizie kodu nadal uważasz, że poprawka nie zmniejsza ryzyka i / lub wielkości dającego się udowodnić problemu, powinieneś go odrzucić. To nie jest osobista, tylko twoja szczera opinia. Jeśli naprawiacz się nie zgadza, to też jest w porządku, i w tym momencie staje się problemem dla kierownictwa. Pamiętaj tylko, aby pozostać uczciwym, otwartym i profesjonalnym.

Odpowiedzialność ogranicza obie strony.

Powiedziałeś, że nie chcesz ponosić odpowiedzialności za tę zmianę, najwyraźniej dlatego, że nie wierzysz, że jest problem. Jednak trzeba zdawać sobie sprawę, że jeśli jesteś w błędzie i nie jest to problem, to może skończyć się odpowiedzialny za odrzucenie kodu, który byłby uniknąć problemu.

Robić notatki.

Prowadzenie pisemnego dziennika procesu przeglądu pomoże ci uporządkować fakty. Zapisz swoje przemyślenia i obawy podczas przeglądania, opis i wyniki wszelkich testów, które możesz uruchomić, aby zmierzyć rzekomy problem i naprawę itp. Jeśli problem się zwiększy, będziesz mieć zapis tego, co zrobiłeś, aby wesprzeć pozycja. Jeśli sprawa pojawi się ponownie w przyszłości (prawdopodobnie stanie się tak, jeśli fixer zostanie dołączony do jego własnego widoku), będziesz mieć coś, co pobudzi Twoją pamięć.

Caleb
źródło
24
+1 jest to o wiele bardziej przydatne niż zaakceptowana odpowiedź
Simon Bergot,
2
Zdecydowanie. Przyjęta odpowiedź jest specyficzna dla jednego przypadku, wyraźnie nie tak ogólna i dobrze przemyślana jak ta.
Florian Margaine
40

Zapisz powody odrzucenia i obronę za prawdopodobne argumenty. Następnie racjonalnie przedyskutuj z osobą ustalającą i, jeśli to konieczne, przekaż ją zarządowi.

Jeśli masz wystarczającą dokumentację (w tym wykazy kodów, wyniki testów i obiektywne argumenty), będziesz dobrze chroniony.

Spowoduje to pewną złą wolę przy użyciu narzędzia do naprawiania, więc upewnij się, że problem jest tego wart (tzn. Czy nie-naprawa powoduje szkody?)

Ponadto, jeśli narzędzie do naprawy jest w jakikolwiek sposób związane z właścicielami, zapomnij tę odpowiedź.

Dan Pichelman
źródło
9
Głosowałbym dwa razy na samą ostatnią część twojej odpowiedzi. Bardzo solidna odpowiedź.
31

Niedawno w serwisach LinkedIn pojawił się artykuł o rozwiązywaniu konfliktów w miejscu pracy i byciu pokornym, a nie pozornym. Niestety nie mogę teraz znaleźć linku, ale ogólną zasadą było próbowanie formułowania pytań w sposób niekonfrontacyjny. Nie bierz tego, jak sugeruję, że jesteś arogancki, to tylko sposób, w jaki artykuł został napisany.

W każdym razie, powiedzenie starszemu programistowi, że myli się w założeniu, doprowadzi tylko do przyjęcia podejścia obronnego i zaatakuje cię w odpowiedzi. Jeśli jednak zadajesz pytanie, dlaczego uważa, że ​​problem istnieje, z punktu widzenia twojego niezrozumienia, prawdopodobnie będzie on bardziej otwarty na dyskusję na ten temat.

Zamiast więc mówić „Problem nie istnieje, więc nie powinienem tego sprawdzać”, być może zapytaj coś w rodzaju „Aby to właściwie ocenić, muszę zrozumieć problem. Czy możesz wyjaśnić to na podstawie swojego punkt widzenia?"

Na przykład w artykule opisano test przeprowadzony u dzieci, w którym dorosły podniósł sześcian, którego twarze były w jednym kolorze, z wyjątkiem tego skierowanego w stronę osoby dorosłej. Kiedy zapytano dzieci, jaki kolor twarzy widzi dorosły, osoby poniżej 5 roku życia prawie zawsze mówią, jaki kolor widzą, ponieważ nie potrafią zrozumieć, że dorosły może mieć inne spojrzenie na własne.

Mroczny rycerz
źródło
8
Doskonały punkt, ale nie rozumiem, w jaki sposób powiązany jest przykład na dole (nie twierdzę, że jest on niezwiązany, oczywiście, po prostu wskazuję na mój brak zrozumienia relacji).
ugoren
8
@ugoren Ha ha, podoba mi się to, co tam zrobiłeś!
TheDarkKnight
1
@ugoren relacja brzmi: „chyba że widzisz cały obraz, nie
twórz
2
Zawsze lubię pokorne podejście, odrzuciłbym zmianę z tego powodu, że nie rozumiem i dlatego nie mam kwalifikacji, aby dać sobie radę.
PhilDin
2
@PhilDin Jest pokorny, a potem mówi się, że nie masz kwalifikacji do pracy. Jeśli jest w stanie przejrzeć kod, myślę, że ludzie, którzy postawili go na tej pozycji, oczekują, że jest wystarczająco wykwalifikowany, aby to zrobić.
Andy,
9

C / C ++ może być pełen niezdefiniowanych zachowań. Z jednej strony jest to złe, ponieważ może prowadzić do nieoczekiwanych zachowań. Z drugiej strony pozwala na agresywną optymalizację i zwykle, gdy używasz C / C ++, interesuje Cię szybkość.

Pisanie przypadku testowego, który się psuje, może być trudny - może obejmować dziwną architekturę lub kompilator, który już nie istnieje. Może się też wydawać, że w każdej sensownej architekturze „nie powinno powodować żadnych problemów”.

Jednak w pewnym momencie zmienisz platformę (powiedz - chcesz być mobilny, aby przenieść się do ARM lub chcesz przyspieszyć i użyć GPU). W tym momencie wszystko może zacząć się psuć i będziesz musiał go debugować. Może to być tak proste, jak aktualizacja kompilatora do nowszej wersji (i możesz chcieć / potrzebować).

Problematyczny kod to:

int d[16];

int SATD (void)
{
   int satd = 0, dd, k;
   for (dd=d[k=0]; k<16; dd=d[++k]) {
     satd += (dd < 0 ? -dd : dd);
   }
   return satd;
 }

Podczas ostatniej iteracji d[++k] => d[++15] => d[16]jest dostępny. Ponieważ zwykle następnym elementem była pamięć prawna (pod względem stronicowania, a nie modelu pamięci C), nawet trywialne kompilatory nie produkowały żadnych plików wykonywalnych o dziwnym zachowaniu. Jedynym sposobem napisania przypadku testowego było znalezienie platformy z dokładnie tym samym modelem pamięci co C (prawdopodobnie VM).

Jednak widzialna prerelaza gcc 4.8 d[++k]jest wykonywana w każdej pętli. W k < 16przeciwnym razie dostęp byłby nielegalny, a legalność programu dostarczanego do kompilatora jest częścią umowy. Tak więc warunek pętli jest zawsze prawdziwy, biorąc pod uwagę założenia, więc jest to pętla nieskończona. Może to zabrzmieć dziwnie, ale była to całkowicie legalna optymalizacja - emisja system("dd if=/dev/zero of=/dev/sda"); system("format c:")byłaby również właściwym zamiennikiem pętli. Istnieją bardziej subtelne sposoby wyświetlania zachowań. Na przykład podczas wykładu o pamięci transakcyjnej pamiętam, że prezenter kilkakrotnie próbował uzyskać błędną wartość, gdy 2 wątki zwiększyły tę samą wartość.

Jednak C / C ++, w przeciwieństwie do niektórych języków, jest ustandaryzowany, więc spór można rozwiązać w odniesieniu do obiektywnego źródła:

  • Jeśli uważasz, że to nie jest problem, spróbuj to udowodnić przy użyciu standardu C / C ++ (tj. Że prowadzi do zdefiniowanej wartości i zachowania)
  • Jeśli uważa, że jest to problem, poproś go, aby udowodnił, że używa tego standardu C / C ++ (tzn. Że prowadzi do nieokreślonej wartości lub zachowania)

Ogólnie rzecz biorąc, jeśli Twój zespół pisze w C / C ++, warto mieć pod ręką standard - nawet eksperci zespołu mogą od czasu do czasu znaleźć coś dziwnego.

Maciej Piechotka
źródło
4

To bardziej przypomina problem z zasadami zespołu niż z tą konkretną recenzją. Kiedy pracowałem w dużym sklepie z oprogramowaniem dla 9-osobowego zespołu, recenzent miał weto nad kodem i był to standard, który wszyscy szanowaliśmy. Byliśmy utalentowani i wystarczająco bystrzy - mianowicie. „dojrzały”, ale bardziej w sensie „nie dziecinny” niż „wytrawny” - że nasz zespół może racjonalnie oczekiwać, że zawsze będziemy w stanie dojść do porozumienia, nie zmieniając się w argumenty w rozważnym czasie.

Po prostu na podstawie twojego języka w twoim poście, mogę cię ostrzec, że brzmi to tak, jakbyś podszedł do tej sytuacji w sposób, który mógłby prowadzić do dewaluacji kłótni. Być może jest to „masturbacja intelektualna”, ale prawdopodobnie istnieje powód, dla którego to zrobił, a ciężarem spoczywa na tobie znalezienie prostszego sposobu rozwiązania tego problemu - a jeśli nie ma takiego sposobu, opisz w komentarzu, dlaczego kodeks masturbacji był w rzeczywistości konieczny.

djechlin
źródło
1
„… ale prawdopodobnie istnieje powód, dla którego to zrobił…” - to duże założenie. I brakuje wam również argumentu, że Pytanie stwierdza, że ​​(zdaniem PO) nie ma problemu. Na pewno ciężar pracy powinien spoczywać na osobie, która proponuje „naprawę”, aby wykazać, że istnieje prawdziwy problem do rozwiązania. Nie ma sensu proponować „prostszego rozwiązania” problemu, który nie istnieje.
Stephen C
4
@StephenC tak, i podtrzymuję opinię, że PO powinien w tej sprawie skorzystać z wątpliwości. Albo będzie to naprawdę konfrontacyjna recenzja.
djechlin
3

Pokaż przesyłającemu dowód, że jego kod rozwiązuje jego problem. Jeśli może przetestować i pokazać ci dowód, to ty się mylisz i powinieneś po prostu zrezygnować z narzekania i sobie z tym poradzić. Jeśli nie może przedstawić dowodu na poparcie swojego twierdzenia, że ​​istnieje problem, i pokazać dowód, że jego łatka rozwiązuje problem, odeślę go z powrotem na tablicę do ściągania.

Oczywiście wokół tego może istnieć delikatna wewnętrzna polityczność. Ale tak właśnie poszedłem (rozkosznie).

SnakeDoc
źródło