Które rzeczy natychmiast dzwonią na alarm, gdy patrzymy na kod? [Zamknięte]

Kilka tygodni temu wziąłem udział w wydarzeniu poświęconym kodzie oprogramowania i jednym z komentarzy było: „Jestem pewien, że wszyscy rozpoznajemy zły kod, gdy go widzimy” i wszyscy kiwnęli mądrze głową bez dalszej dyskusji.

Takie rzeczy zawsze mnie martwią, ponieważ istnieje taki truizm, że wszyscy myślą, że są ponadprzeciętnym kierowcą. Chociaż wydaje mi się, że potrafię rozpoznać zły kod, chciałbym dowiedzieć się więcej o tym, co inni uważają za zapach, ponieważ rzadko jest on szczegółowo omawiany na blogach i tylko w kilku książkach. W szczególności myślę, że byłoby interesujące usłyszeć o wszystkim, co pachnie kodem w jednym języku, ale nie w innym.

Zacznę od łatwego:

Kod w kontroli źródła, który ma duży odsetek skomentowanych kodów - dlaczego on tam jest? czy miał zostać usunięty? czy to w połowie ukończony utwór? może nie powinno to być komentowane i zostało zrobione tylko wtedy, gdy ktoś coś testował? Osobiście uważam, że tego rodzaju rzeczy są naprawdę denerwujące, nawet jeśli jest to tylko dziwna linia tu i tam, ale kiedy widzisz duże bloki przeplatane resztą kodu, jest to całkowicie niedopuszczalne. Zazwyczaj jest to również wskazanie, że reszta kodu może mieć również wątpliwą jakość.

/* Fuck this error */

Zazwyczaj znajduje się w nonsensownym try..catchbloku, zwykle przyciąga moją uwagę. Prawie tak dobrze jak /* Not sure what this does, but removing it breaks the build */.

Jeszcze kilka rzeczy:

• Wiele zagnieżdżonych ifinstrukcji złożonych
• Bloki try-catch, które są używane do regularnego ustalania logiki
• Funkcje z nazw rodzajowych process, data, change, rework,modify
• Sześć lub siedem różnych stylów stężeń w 100 liniach

Właśnie znalazłem:

/* Stupid database */
$conn = null;
while(!$conn) {
    $conn = mysql_connect("localhost", "root", "[pass removed]");
}
/* Finally! */
echo("Connected successfully.");

Racja, ponieważ konieczność brutalnej siły połączeń MySQL to właściwy sposób na robienie różnych rzeczy. Okazało się, że w bazie danych wystąpiły problemy z liczbą połączeń, więc przekroczono limit czasu. Zamiast tego debugować, po prostu próbowali raz po raz, aż zadziałało.

Główną czerwoną flagą dla mnie są zduplikowane bloki kodu, ponieważ pokazuje, że osoba albo nie rozumie podstaw programowania, albo była zbyt wystraszona, aby dokonać odpowiednich zmian w istniejącej bazie kodu.

Zwykłem też liczyć brak komentarzy jako czerwoną flagę, ale ostatnio pracowałem nad bardzo dobrym kodem, bez komentarzy.

Kod, który próbuje pokazać, jak sprytny jest programista, mimo że nie dodaje żadnej rzeczywistej wartości:

x ^= y ^= x ^= y;

• 20 000 funkcji linii (przesada). Każda funkcja, która wymaga więcej niż kilku ekranów, wymaga ponownego faktorowania.

• W tym samym wierszu pliki klas, które wydają się trwać wiecznie. Prawdopodobnie istnieje kilka pojęć, które można by podzielić na klasy, które wyjaśniłyby cel i funkcję oryginalnej klasy i prawdopodobnie gdzie jest używana, chyba że wszystkie są metodami wewnętrznymi.

• zmienne nieopisowe, nietrywialne lub zbyt wiele trywialnych zmiennych nieopisowych. Sprawiają one, że dedukowanie tego, co się dzieje, jest zagadką.

{ Let it Leak, people have good enough computers to cope these days }

Gorzej jest z biblioteki komercyjnej!

Komentarze, które są tak szczegółowe, że gdyby istniał angielski kompilator, skompilowałby się i działałby idealnie, ale nie opisuje niczego, czego nie koduje.

//Copy the value of y to temp.
temp = y;
//Copy the value of x to y.
y = x;
//Copy the value of temp to x.
x = temp;

Ponadto komentarze do kodu, które można było usunąć, gdyby kod był zgodny z kilkoma podstawowymi wytycznymi:

//Set the age of the user to 13.
a = 13;

Kod generujący ostrzeżenia po kompilacji.

Funkcje z numerami w nazwie zamiast nazw opisowych , takie jak:

void doSomething()
{
}

void doSomething2()
{
}

Proszę, niech nazwy funkcji coś znaczą! Jeśli doSomething i doSomething2 robią podobne rzeczy, użyj nazw funkcji, które różnicują różnice. Jeśli doSomething2 jest wyłomem w funkcji doSomething, nazwij go swoją funkcjonalnością.

Magiczne liczby lub magiczne struny.

   if (accountBalance>200) { sendInvoice(...); }

   salesPrice *= 0.9;   //apply discount    

   if (invoiceStatus=="Overdue") { reportToCreditAgency(); }

• Może nie najgorszy, ale wyraźnie pokazuje poziom implementatorów:

  if(something == true) 

• Jeśli język ma konstrukcję pętli for lub iteratora, wówczas użycie pętli while pokazuje również poziom zrozumienia języka przez implementatora:

  count = 0; 
  while(count < items.size()){
     do stuff
     count ++; 
  }
  
  for(i = 0; i < items.size(); i++){
    do stuff 
  }
  //Sure this is not terrible but use the language the way it was meant to be used.

• Słaba pisownia / gramatyka w dokumentacji / komentarzach zjada prawie tyle samo, co sam kod. Powodem tego jest to, że kod przeznaczony był do czytania przez ludzi i uruchamiania maszyn. Dlatego używamy języków wysokiego poziomu, jeśli twoja dokumentacja jest trudna do przejścia, to zapobiegawczo formuję negatywną opinię o bazie kodu, nie patrząc na nią.

Ten, który od razu zauważam, to częstotliwość głęboko zagnieżdżonych bloków kodu (if's, while, itp.). Jeśli kod często schodzi głębiej niż na dwa lub trzy poziomy, jest to oznaką problemu projektowego / logicznego. A jeśli dojdzie do głębokości 8 gniazd, niech będzie dobry powód, aby go nie rozpadać.

Oceniając program ucznia, czasami potrafię powiedzieć w „mrugającym” momencie. Oto natychmiastowe wskazówki:

• Słabe lub niespójne formatowanie
• Więcej niż dwie puste linie z rzędu
• Niestandardowe lub niespójne konwencje nazewnictwa
• Powtarzany kod, im więcej dosłownych powtórzeń, tym silniejsze ostrzeżenie
• To, co powinno być prostym fragmentem kodu, jest zbyt skomplikowane (na przykład sprawdzanie argumentów przekazywanych do main w zawiły sposób)

Rzadko moje pierwsze wrażenia są nieprawidłowe, a te dzwonki ostrzegawcze mają rację przez około 95% czasu . Z jednym wyjątkiem nowy student języka posługiwał się stylem z innego języka programowania. Wkopanie się i ponowne przeczytanie ich stylu w języku innego języka usunęło dla mnie dzwonki alarmowe, a student uzyskał pełne uznanie. Ale takie wyjątki są rzadkie.

Rozważając bardziej zaawansowany kod, oto inne moje ostrzeżenia:

• Obecność wielu klas Java, które są jedynie „strukturami” do przechowywania danych. Nie ma znaczenia, czy pola są publiczne czy prywatne i używają getterów / setterów, nadal prawdopodobnie nie jest to część dobrze przemyślanego projektu.
• Klasy o złych nazwach, takie jak po prostu przestrzeń nazw i brak prawdziwej spójności w kodzie
• Odniesienie do wzorców projektowych, które nawet nie są używane w kodzie
• Puste programy obsługi wyjątków bez wyjaśnienia
• Kiedy podciągam kod w Eclipse, setki żółtych „ostrzeżeń” wykładają kod, głównie z powodu nieużywanego importu lub zmiennych

Pod względem stylu generalnie nie lubię widzieć:

• Komentarze Javadoc, które tylko odbijają kod

Są to tylko wskazówki do złego kodu. Czasami coś, co może się wydawać złym kodem, tak naprawdę nie jest, ponieważ nie znasz intencji programisty. Na przykład może istnieć dobry powód, dla którego coś wydaje się zbyt skomplikowane - może istnieć inna uwaga w grze.

Osobiste faworyta / peeve: generowane przez IDE nazwiska, które zostają zatwierdzone. Jeśli TextBox1 jest ważną i ważną zmienną w twoim systemie, czeka Cię kolejna rzecz, którą można przejrzeć kod.

Całkowicie nieużywane zmienne , szczególnie gdy zmienna ma nazwę podobną do używanych zmiennych.

Wiele osób wspomniało:

//TODO: [something not implemented]

Chociaż chciałbym, żeby rzeczy zostały zaimplementowane, przynajmniej zrobili notatkę. To, co uważam za gorsze, to:

//TODO: [something that is already implemented]

Todo są bezwartościowe i mylące, jeśli nigdy nie zawracasz sobie głowy ich usunięciem!

Metoda, która wymaga ode mnie przewinięcia w dół, aby przeczytać wszystko.

Koniunkcje w nazwach metod:

public void addEmployeeAndUpdatePayrate(...) 


public int getSalaryOrHourlyPay(int Employee) ....

Wyjaśnienie: Powodem tego jest to, że dzwonki alarmowe wskazują, że metoda prawdopodobnie narusza zasadę pojedynczej odpowiedzialności .

Łączenie oczywiście kodu źródłowego GPL z komercyjnym programem o zamkniętym źródle.

Nie tylko powoduje to bezpośredni problem prawny, ale z mojego doświadczenia wynika, że ​​zwykle oznacza to niedbalstwo lub brak zainteresowania, co znajduje odzwierciedlenie również w innym miejscu kodu.

Język agnostyczny:

• TODO: not implemented
• int function(...) { return -1; } (to samo co „nie zaimplementowano”)
• Zgłaszanie wyjątku z niecodziennego powodu.
• Nadużywanie lub niespójne stosowanie 0, -1lub nulljako wyjątkowe wartości zwracane.
• Twierdzenia bez przekonującego komentarza mówiące, dlaczego nigdy nie powinno zawieść.

Specyficzne dla języka (C ++):

• Makra C ++ pisane małymi literami.
• Zmienne statyczne lub globalne C ++.
• Niezainicjowane lub nieużywane zmienne.
• Wszystko, array newco najwyraźniej nie jest bezpieczne dla RAII.
• Każde użycie tablicy lub wskaźnika, które najwyraźniej nie jest bezpieczne dla granic. Obejmuje to printf.
• Każde użycie niezainicjowanej części tablicy.

Specyficzne dla Microsoft C ++:

• Wszelkie nazwy identyfikatorów, które kolidują z makrem już zdefiniowanym przez dowolny plik nagłówka Microsoft SDK.
• Ogólnie rzecz biorąc, każde użycie Win32 API jest dużym źródłem dzwonków alarmowych. Zawsze miej otwartą MSDN i sprawdzaj definicje argumentów / zwracanych wartości w razie wątpliwości. (Edytowane)

Specyficzne dla C ++ / OOP:

• Dziedziczenie implementacji (konkretnej klasy), w której klasa nadrzędna ma metody wirtualne i nie-wirtualne, bez wyraźnego oczywistego rozróżnienia koncepcyjnego między tym, co powinno / nie powinno być wirtualne.

Dziwny styl wcięcia.

Jest kilka bardzo popularnych stylów, a ludzie zabiorą tę debatę do grobu. Ale czasami widzę, że ktoś używa naprawdę rzadkiego, a nawet domowego stylu wcięcia. To znak, że prawdopodobnie nie kodowali z nikim innym niż sobą.

Używanie dużej liczby bloków tekstowych zamiast wyliczeń lub zmiennych zdefiniowanych globalnie.

Niedobrze:

if (itemType == "Student") { ... }

Lepszy:

private enum itemTypeEnum {
  Student,
  Teacher
}
if (itemType == itemTypeEnum.Student.ToString()) { ... }

Najlepsza:

private itemTypeEnum itemType;
...
if (itemType == itemTypeEnum.Student) { ... }

Słabo wpisane parametry lub zwracane wartości metod.

public DataTable GetEmployees() {...}

public DateTime getHireDate(DataTable EmployeeInfo) {...}

public void FireEmployee(Object EmployeeObjectOrEmployeeID) {...}

• Wiele operatorów trójskładnikowych połączonych razem, więc zamiast przypominać if...elseblok, staje się if...else if...[...]...elseblokiem
• Długie nazwy zmiennych bez podkreślników i obudowy wielbłąda. Przykład z jakiegoś kodu, który wyciągnąłem:$lesseeloginaccountservice
• Setki linii kodu w pliku, bez komentarzy lub z niewielkimi komentarzami, a kod jest bardzo nieoczywisty
• Zbyt skomplikowane ifstwierdzenia. Przykład z kodu: do if (!($lessee_obj instanceof Lessee && $lessee_obj != NULL))którego przejechałemif ($lessee_obj == null)

Zapach kodu: nieprzestrzeganie najlepszych praktyk

Takie rzeczy zawsze mnie martwią, ponieważ istnieje taki truizm, że wszyscy myślą, że są ponadprzeciętnym kierowcą.

Oto krótka wiadomość dla ciebie: 50% światowej populacji jest poniżej średniej inteligencji. Ok, więc niektórzy ludzie mieliby dokładnie przeciętną inteligencję, ale nie bądźmy wybredni. Ponadto jednym z bocznych skutków głupoty jest to, że nie możesz rozpoznać swojej głupoty! Po połączeniu tych oświadczeń rzeczy nie wyglądają tak dobrze.

Które rzeczy natychmiast dzwonią na alarm, gdy patrzymy na kod?

Wspomniano wiele dobrych rzeczy i ogólnie wydaje się, że nie przestrzega się najlepszych praktyk to zapach kodu.

Najlepsze praktyki zwykle nie są wymyślane losowo i często istnieją z jakiegoś powodu. Wiele razy może to być subiektywne, ale z mojego doświadczenia są w większości uzasadnione. Przestrzeganie najlepszych praktyk nie powinno stanowić problemu, a jeśli zastanawiasz się, dlaczego są takie, jakie są, badaj je, zamiast ignorować i / lub narzekać - może to uzasadnione, a może nie.

Jednym z przykładów najlepszej praktyki może być stosowanie curlies z każdym blokiem if, nawet jeśli zawiera on tylko jedną linię:

if (something) {
    // whatever
}

Może nie uważasz, że jest to konieczne, ale niedawno przeczytałem , że jest to główne źródło błędów. Zawsze używano nawiasów również omówiono na temat przepełnienia stosu , a sprawdzanie, czy instrukcje if mają nawiasy, jest również regułą w PMD , statycznym analizatorze kodu dla Javy.

Pamiętaj: „Ponieważ to najlepsza praktyka” nigdy nie jest akceptowalną odpowiedzią na pytanie „dlaczego to robisz?” Jeśli nie potrafisz wyrazić, dlaczego coś jest najlepszą praktyką, to nie jest to najlepsza praktyka, to przesąd.

Komentarze, które mówią: „dzieje się tak, ponieważ konstrukcja podsystemu„ Froz ”jest całkowicie błędna”.

To trwa cały akapit.

Wyjaśniają, że musi nastąpić następujący refaktor.

Ale tego nie zrobiłem.

Być może powiedziano im, że ich szef nie może wówczas zmienić z powodu problemów z czasem lub kompetencjami, ale może dlatego, że ludzie są małostkowi.

Jeśli przełożony uważa, że ​​j. Losowo. programista nie może dokonać refaktoryzacji, wówczas powinien to zrobić przełożony.

W każdym razie tak się dzieje, wiem, że kod został napisany przez podzielony zespół, z możliwą polityką władzy, i nie zmienili projektów podsystemów.

Prawdziwa historia. To mogło zdarzyć się Tobie.

Czy ktoś może pomyśleć o przykładzie, w którym kod powinien zgodnie z prawem odnosić się do pliku bezwzględną ścieżką?

Łapanie ogólnych wyjątków:

try {

 ...

} catch {
}

lub

try {

 ...

} catch (Exception ex) {
}

Nadużycie regionu

Zwykle użycie zbyt wielu regionów wskazuje mi, że twoje klasy są za duże. Jest to flaga ostrzegawcza, która sygnalizuje, że powinienem bardziej zbadać ten fragment kodu.

Konwencje nazewnictwa klas, które pokazują słabe zrozumienie abstrakcji, którą próbują stworzyć. Lub które wcale nie definiują abstrakcji.

Ekstremalny przykład przychodzi mi na myśl w klasie VB, którą kiedyś widziałem, która miała tytuł Datai miała ponad 30 000 linii ... w pierwszym pliku. Była to klasa częściowa podzielona na co najmniej pół tuzina innych plików. Większość metod dotyczyła przechowywanych procesów o nazwach podobnych do FindXByYWithZ().

Nawet z mniej dramatycznymi przykładami, jestem pewien, że wszyscy po prostu „zrzuciliśmy” logikę do źle pomyślanej klasy, nadając jej całkowicie ogólny tytuł, i żałowałem tego później.

Funkcje, które przywracają podstawową funkcjonalność języka. Na przykład, jeśli kiedykolwiek zobaczysz w JavaScript metodę „getStringLength ()” zamiast wywołania właściwości „.length” ciągu, wiesz, że masz kłopoty.

#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...

Oczywiście bez jakiejkolwiek dokumentacji i okazjonalnych zagnieżdżonego #defines