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

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?

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.

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ęć.

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ź.

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.

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.

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.

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).