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

98

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

FinnNk
źródło
61
Czasami spotykam ludzi, którzy komentują kod, meldują się i mówią: „Mogę go potrzebować w przyszłości - jeśli go teraz usunę, stracę go”. Muszę przeciwstawić się „Er, ... ale po to jest kontrola źródła”.
talonx,
6
Czasami, szczególnie podczas optymalizacji, przydatne jest pozostawienie starego kodu jako komentarza, abyś wiedział, co zastępuje niejasny zoptymalizowany kod. Pomyśl o pozostawieniu 3-liniowej zamiany z tempem na miejscu, zastępując ją jednowierszowym bitem zamiany. (Chociaż nie widzę potrzeby korzystania z zamiany jednowierszowej - NIGDY, chyba że rozmiar programu ma kluczowe znaczenie.)
Chris Cudmore,
4
Utrzymuję / usuwam kod napisany przez jednego z naszych inżynierów, który napisał kilka przydatnych rzeczy, ale przyznaje, że nie jest programistą. Kiedy konsoliduję rzeczy, skomentuję jego stary kod, a następnie przejrzymy zmiany i pokażę mu, jak zastąpiłem jego czymś mniejszym / wydajniejszym / łatwiejszym do zrozumienia. Następnie usuwam te bloki, a potem je sprawdzam. Posiadanie starego kodu ma zalety, ponieważ widzi, jak można to zrobić w prostszy sposób i pamiętam, dlaczego zmieniłem to, kiedy rozmawiamy.
Tin Man,
8
Zostawiam rzeczy, które „mogą być użyte” dla 1 zatwierdzenia, a jeśli coś się nie zepsuje lub nie zostanie znaleziona potrzeba, zostanie usunięta przy następnym zatwierdzeniu.
Paul Nathan
24
Hmm printf("%c", 7)zazwyczaj dzwoni mi dzwonek alarmowy. ;)

Odpowiedzi:

128
/* 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.

Josh K.
źródło
19
Gdybym tylko mógł głosować za 6 razy! Wszystkie dobre przykłady. Nie lubię też aroganckich / śmiesznych komentarzy (zwłaszcza jeśli zawierają przekleństwa) - może to być trochę zabawne, kiedy je czytasz po raz pierwszy, ale bardzo szybko się starzeją (a więc rozpraszają).
FinnNk
5
Podoba mi się twój przykład, chociaż powiedziałbym, że w pewnych kontekstach, wiele zagnieżdżonych, jeśli instrukcje są nieuniknione. Przy dużej logice biznesowej kod może być nieco mylący, ale jeśli sama firma jest myląca na początku, uproszczenie kodu byłoby mniej dokładne modelowanie procesu. Jak powiedział Einstein: „Wszystko powinno być tak proste, jak to możliwe, a nie odrobinę prostsze”.
Morgan Herlocker,
2
@Prof Plum - jaki możesz podać przykład? Zwykle alternatywą dla wielu zagnieżdżonych jest podzielenie go na (wiele) metod. Młodsi programiści zwykle unikają tego, jakby było to mniej pożądane niż w przypadku if; ale zwykle po naciśnięciu mówią „jeśli zrób to w mniejszej liczbie wierszy”. Potrzebuje kogoś, kto jest pewien OOP, aby wkroczyć i przypomnieć mu, że mniej wierszy! = Lepszy kod.
STW,
2
@STW To dobra uwaga, powiedziałbym jednak, że zależy to od głębokości zagnieżdżenia. Z pewnością zgodziłbym się, że coś głębszego niż trzy gniazda często wymaga refaktoryzacji, ponieważ będzie dość owłosione. Cytowanie ubezpieczeń jest jednak dobrym przykładem, w którym wiele gniazd może całkiem dobrze modelować rzeczywisty świat. Z wyjątkami od niektórych stawek / premii, instrukcja dosłownie brzmi „jeśli jest to właściwość i jeśli ma minimalną stawkę poniżej 5,6 i jeśli jest w NC i jeśli ma łódź na terenie, zrób to i taki." z wieloma innymi opcjami.
Morgan Herlocker,
4
@ josh, jeśli „oni” są kolegami, zastanowiłbym się, dlaczego nie powiedziałeś „my” ...
104

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.

Ben Hughes
źródło
1
+1: Widziałem trochę kodu od kolegi, który reklamował się jako ekspert od Linuksa, który napisał prostą aplikację do zapętlania jako jedną długą funkcję, cały czas w main (). Yikes.
KFro
4
@ Kro, to rozwijanie pętli. To właśnie robią kompilatory cały czas :) BARDZO wydajne!
3
@Thorbjorn: Czasami trzeba trochę pomóc kompilatorowi; w końcu jesteś mądrym człowiekiem, a on jest po prostu głupim komputerem.
yatima2975,
3
Kolejny powód, który widziałem: konsultantowi zapłacono za jak najszybsze wdrożenie tej funkcji (dlatego oczywiście brakuje też testów i dokumentacji). Kopiowanie / wklejanie jest szybsze niż myślenie o tym, jak zrobić to dobrze.
LennyProgrammers,
4
Unikanie powielania kodu może być obsesją. W języku takim jak C ++ nie zawsze jest tak łatwe, jak powinno być, rozróżnianie różnych części, ale wciąż ma solidny i wydajny kod. Czasami bardziej praktyczna jest opcja wycinania i wklejania. Można również zastosować zasadę optymalizacji - wycinanie i wklejanie może zapewnić szybkie i łatwe rozwiązanie, które można później zmienić w razie potrzeby. Być może oszczędzasz sobie późniejszy ból głowy związany z konserwacją, ale wiesz na pewno, że teraz unikasz opóźnień.
Steve314,
74

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

x ^= y ^= x ^= y;
Rei Miyasaka
źródło
12
Wow, to jest o wiele bardziej czytelne niżswap(x, y);
JBRWilkinson
8
jeśli xiy są wskaźnikami, a to przypisanie ma miejsce w rozsądnym okresie czasu, mocno psuje konserwatywne śmieciarze, takie jak Boehm GC.
SingleNegationElimination
12
Nawiasem mówiąc, ten kod jest niezdefiniowany w C i C ++ z powodu wielu zmian bez pośredniego punktu sekwencji.
fredoverflow
9
Patrząc na to, jedyne, co przyszło mi do głowy, to emotikony:^_^
Darien,
62
  • 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ą.

Dominique McDonnell
źródło
9
W miarę możliwości ograniczam funkcje do 1 ekranu.
Matt DiTrolio,
20
1 ekran to nawet odcinek. Zaczynam czuć się brudny po około 10 liniach.
Bryan Rowe
54
Dobra, zamierzam wyrazić opinię, która może być niepopularna. Mówię, że zapachem kodu jest pisanie funkcji, które są atomowymi procesami od góry do dołu, które są podzielone na osobne funkcje, ponieważ programista trzyma się niektórych „funkcji, które powinny być krótkie”, kultywowania ładunków. Funkcje powinny być łamane wzdłuż FUNKCJONALNYCH linii, nie tylko dlatego, że powinny być mitycznymi „odpowiednimi rozmiarami”. Dlatego nazywają się FUNKCJE.
Dan Ray
7
@ Dan, Funkcje nie powinny być krótkie ze względu na to, że są krótkie, ale jest tylko tyle informacji, które możesz trzymać w głowie jednocześnie. Może mam mały mózg, bo dla mnie limit to kilka ekranów :). Podział funkcji na wiele funkcji, gdy zaczynają testować tę granicę, jest konieczny, aby uniknąć błędów. Z jednej strony zapewnia enkapsulację, dzięki czemu możesz myśleć na wyższym poziomie, z drugiej zaś ukrywa to, co się dzieje, dlatego trudniej jest zrozumieć, jak działa ta funkcja. Uważam, że należy rozdzielić funkcje, aby poprawić czytelność, a nie dopasować do „idealnej długości”.
Dominique McDonnell,
6
@Dominic, @ Péter, myślę, że cała nasza trójka mówi to samo. Kiedy jest dobry powód, aby kod podzielić na mniejsze funkcje, jestem za tym. To, co odrzucam, to krótkość ze względu na niedociągnięcie w projektowaniu funkcji. Wiesz, stos wywołań jest trzy razy dłuższy, niż powinien, ale przynajmniej te funkcje są krótkie. Wolę debugować wysoką funkcję, która robi jedną rzecz dobrze i wyraźnie, niż gonić ścieżkę wykonania przez tuzin połączonych funkcji, z których każda jest wywoływana tylko z poprzedniej.
Dan Ray
61
{ Let it Leak, people have good enough computers to cope these days }

Gorzej jest z biblioteki komercyjnej!

Naprawdę etyczne
źródło
32
To nie dzwoni dzwonkami alarmowymi. Praktycznie kopie cię między nogi.
Steven Evers
15
Córka jest uzależniona od metamfetaminy. Kogo to obchodzi, przynajmniej ona nie stanie się otyła. :: westchnienie ::
Evan Plaice,
17
Kiedy znajduję się w trudnej sytuacji, przychodzi do mnie matka buntu. Mówiąc słowa mądrości, niech wycieknie. POZWÓL TO WYCIEKU. POZWÓL TO PRZECIEĆ oh POZWÓL TO WYCIEĆ. Jeśli tylko raz wycieknie, to nie jest leaaaaaak. (jeśli przeczytałeś do tej pory, +1). Naprawdę muszę rozważyć bezkofeinową.
Tim Post
13
„Gdy zbliża się termin ostateczny, przecieki są wszystkim, co widzę, gdzieś ktoś szepcze:„ Napisz w C ...... eeeeee !!! ””
chiurox
9
Dawno, dawno temu istniały dwa systemy operacyjne. Jeden wyciekł i rozbił się, gdyby działał przez ponad 49 dni, drugi był idealny i działałby na zawsze. Jeden z nich został wydany w 1995 roku na wielkim fanfairu i był używany przez miliony ludzi - drugi nigdy nie został wysłany, ponieważ wciąż sprawdzają, czy jest wolny od błędów. Istnieje różnica między filozofią a inżynierią.
Martin Beckett,
53

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;
Rei Miyasaka
źródło
15
Nazywa się COBOL :-)
Gajusz
26
Drugi przypadek nie jest najgorszy. Najgorsze jest: / * Ustaw wiek użytkownika na 13 * / a = 18;
PhiLho,
4
@PhiLho - nie, jeszcze gorzej jest, gdy brakuje /od z */, więc cały kod do końca następnego */jest ignorowany. Na szczęście podświetlanie składni sprawia, że ​​takie rzeczy są obecnie rzadkością.
Steve314,
3
Gorzej, adla user_age? Naprawdę?
glasnt
2
Kiedyś utrzymywałem standardowy dokument kodu u poprzedniego pracodawcy, którego jedna sekcja zawierała odpowiednie komentarze. Mój ulubiony przykład pochodzi z MSDN:i = i + 1; //increment i
Michael Itzoe
42

Kod generujący ostrzeżenia po kompilacji.

Rei Miyasaka
źródło
1
Istnieje kandydat do dodania opcji kompilatora „Wszystkie ostrzeżenia jako błędy” do pliku makefile / projektu.
JBRWilkinson
3
Myślę, że może to być przydatne, jeśli masz projekt z wieloma osobami, którym po prostu nie ufasz - choć gdybym przyłączył się do projektu, w którym ustawiono tę opcję, to samo w sobie zmartwiłoby mnie, jak inny programiści są.
Rei Miyasaka,
1
Nie zgadzam się z tym. Niektóre ostrzeżenia kompilatora (np. Porównanie podpisanego i niepodpisanego, gdy wiadomo , że obie wartości są niepodpisane, nawet jeśli typy są różne) są lepsze niż zaśmiecanie kodu niepotrzebnymi rzutowaniami. Jeśli zmniejszę kod za pomocą przenośnej liczby całkowitej ze znakiem, którą funkcja modyfikuje tylko wtedy, gdy liczba całkowita ma wartość bez znaku, zrobię to.
Tim Post
13
Wolę zaśmiecać mój kod zbędnym zbędnym kodem (unsigned int)niż zaśmiecać moje listy ostrzeżeń / błędów łagodnymi ostrzeżeniami. Nie chciałbym, żeby lista ostrzeżeń stała się martwym punktem. To również znacznie więcej pita wyjaśniający do innych ludzi, dlaczego jesteś ignorując ostrzeżenie, niż jest, aby wyjaśnić, dlaczego robisz obsady naturalny intsdo unsigned ints.
Rei Miyasaka,
Czasami musisz pracować z interfejsem API, który wyrzuca błędy, cokolwiek robisz. Klasyczne przykłady pokazują, że interfejs API jest definiowany w kategoriach rzeczy, które są zepsute przez projekt (niektóre stare stałe ioctl () były podobne, a czasami programiści systemu operacyjnego nalegają na użycie niewłaściwego typu w swoich nagłówkach) lub gdzie przestali działać bez pozostawiając dobry zamiennik (dziękuję, Apple).
Donal Fellows
36

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

Wonko przy zdrowych zmysłach
źródło
Podobnie @Parm dla SQL
Dave
2
+1 - Enter mshtml- łamie mi oczy :(
Kyle Rozendo
3
Wyjątkiem byłby kod GUI. Na przykład, jeśli formularz ślimak-mail miał dwa adresy; adres1 i adres2 jest bardziej rozsądny niż adres i adres alternatywny. Fałszywe etykiety, które są tylko statyczne, są również rozsądnym wyjątkiem.
Evan Plaice,
@Evan - dość uczciwie, chociaż bardziej rozróżniałem funkcjonalność.
Wonko the Sane
1
+1 - Widziałem to nawet jako metodę kontroli pseudo-wersji.
EZ Hart
36

Magiczne liczby lub magiczne struny.

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

   salesPrice *= 0.9;   //apply discount    

   if (invoiceStatus=="Overdue") { reportToCreditAgency(); }
JohnFx
źródło
4
Nie tak źle w przypadku dwóch drugich, przynajmniej wyjaśniono rabat, a „Zaległe” jest intuicyjne. Z 200drugiej strony ...
Tarka,
9
@Slokun - Intuicyjny nie chodzi o tyle o łatwość utrzymania i kruchość. Na przykład zastanów się, co się stanie, gdy kwota rabatu ulegnie zmianie, a 0,9 zostanie zakodowane na stałe w sześciu różnych miejscach. Ponadto używanie ciągów zamiast stałych / wyliczeń powoduje problemy w językach z ciągami rozróżniającymi małe i wielkie litery.
JohnFx,
+1 Właśnie spędziłem zbyt dużo czasu na debugowaniu problemu, który okazał się być spowodowany linią „timeout = 15;” pochowany w programie.
aubreyrhodes
Myślę, że ostatni jest czasem w porządku, w zależności od tego, skąd pochodzą dane do fakturyStatus. Jeśli pochodzi on z publicznego interfejsu API, który zwraca właśnie dekodowany kod JSON, sprawdzanie zgodności z zakodowaną stałą String jest prawdopodobnie w porządku. Zgadzam się jednak, że „200” i „0.9” to tylko stałe magiczne i nie powinny być w ten sposób zakodowane na stałe. Nawet jeśli są one używane tylko w tym jednym miejscu, konserwacja jest łatwiejsza, jeśli zdefiniujesz je osobno w sekcji konfiguracji, zamiast umieszczać je w kodzie logicznym. A jeśli są używane w wielu miejscach, konserwacja staje się znacznie łatwiejsza.
Ben Lee,
36
  • 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ą.

Chris
źródło
29

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

Grandmaster B.
źródło
6
Wiem, że niektórzy ludzie nauczyli się, że każda metoda powinna mieć tylko jedno returnstwierdzenie, ale gdy powoduje ona ponad 6 poziomów zagnieżdżenia, to myślę, że wyrządza o wiele więcej szkody niż pożytku.
Darien,
28

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.

Macneil
źródło
Nie rozumiem, jak użycie stylu z jednego języka w innym jest poważnym błędem (2, 4, 8 spacji na tiret ...). Jeśli uczeń ma niewiele innych stylów do naśladowania, nie ma nic złego w byciu konsekwentnym. Jako równiarka widzisz miliard programów, więc jesteś na drugim końcu tego spektrum, ale to nie jest powód, aby całkowicie odrzucać inny (ale spójny) styl.
Nick T
18
Nie widzę nic złego w klasach, które po prostu agregują dane - tj. Struktury. Właśnie takimi obiektami do przesyłania danych (DTO) są i mogą uczynić kod znacznie bardziej czytelnym niż powiedzmy, np. Przekazując 20 parametrów do metody.
Nemi
1
Twój komentarz na temat struktur jest natychmiastowy. Jest w porządku, gdy dane są w najbardziej surowej formie i nie zostaną w żaden sposób zmodyfikowane. Ale w 95% przypadków powinieneś mieć w klasie pewne funkcje do formatowania / obsługi danych. Właśnie przypomniałem sobie część mojego kodu, który zasadniczo używa tego wzorca i należy go ulepszyć.
DisgruntledGoat
2
+1 za niespójny styl i dodatkowe łamanie linii (widziałem losowe wcięcia, brak wcięcia, losowe i zmieniające się konwencje nazewnictwa i więcej - i to w kodzie produkcyjnym!). Jeśli nie możesz nawet zadać sobie trudu, aby to dobrze zrobić, to co jeszcze nie możesz zrobić?
Dean Harding
1
Szukam linii deklaracji metody, która jest wcięta zbyt daleko w stosunku do ciała. Jest to znak, który skopiowali i wkleili z pliku innej osoby.
Barry Brown,
25

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.

Wyatt Barnett
źródło
25

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

Krzyż
źródło
21

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!

Morgan Herlocker
źródło
1
Właśnie przeszedłem ćwiczenie polegające na tym, że muszę przygotować raport ze wszystkich TODO w naszym wydanym produkcie, a także plan ich rozmieszczenia. Prawie połowa okazała się przestarzała.
AShelly,
1
-1 Komentarze TODO są używane w MS Visual Studio do śledzenia części projektu, które nadal wymagają pracy. IE, IDE prowadzi listę, która śledzi TODO, więc możesz łatwo kliknąć na nie i przejść bezpośrednio do linii, w której TODO istnieje. Wolałbym, aby TODO były umieszczone wyraźnie, aby zobaczyć, jakie prace należy jeszcze wykonać. Zobacz dotnetperls.com/todo .
Evan Plaice,
3
@Evan Plaice: Wow, masz na myśli, że VS IDE rozpoznaje, kiedy coś zaimplementowałeś i usuwa //TODOkomentarz? Niesamowite!
JBRWilkinson
4
@Prof Plum Dlaczego nie stworzyć polityki, w której osoba odpowiedzialna za TODO umieszcza swoje nazwisko w komentarzu. W ten sposób, jeśli zostaną jakieś resztki
Evan Plaice,
3
Lepszy plan niż // TODO , użyj swojego narzędzia do śledzenia błędów, po to jest to!
SingleNegationElimination
20

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

BradB
źródło
14
Hmm ... to zależy od tego, co jest wdrażane. Nie byłoby nieuzasadnione, aby miało to miejsce przy wdrażaniu skomplikowanych algorytmów, ponieważ w taki sposób są one definiowane. Oczywiście, jeśli większość metod działa w ten sposób, jest to czerwona flaga.
Billy ONeal
9
W ogólnym zarysie nie zgadzam się z tym, że marnowanie czasu na ciągłe refaktoryzowanie, tak aby wszystko pasowało do tego rodzaju narzuconych reguł, faktycznie podnosi całkowity koszt projektu.
Anonimowy typ
1
Nie zgadzam się, że ta zasada może zwiększyć całkowity koszt projektu, ale wydaje mi się, że jest to subiektywne, ponieważ byłoby zależne od dewelopera (ów). Jeśli podczas opracowywania będziesz przestrzegać ogólnej zasady „rozdzielania problemów”, wówczas przefakturowanie byłoby mniejszym zadaniem, jeśli zdecydujesz się to zrobić. Należy rozważyć, ile by to kosztowało trzy lata później, gdy programiści, którzy nie kodowali oryginalnego projektu, próbują naprawić kilka metod z ponad 300 liniami kodu? Ile to kosztuje?
BradB
18
Bardziej denerwuje mnie przewijanie w prawo niż przewijanie w dół. Biała spacja jest „darmowa”.
Wonko the Sane
4
Wolę przewijać niż pomijać cały plik (lub wiele plików), aby dowiedzieć się, co robi kod.
TMN
20

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 .

JohnFx
źródło
Hmmm ... jeśli nazwa funkcji dokładnie określa jej działanie, to się nie zgadzam. Jeśli metoda ma dwie oddzielne rzeczy, które lepiej byłoby rozdzielić, a następnie I może zgodzić się, w zależności od kontekstu.
Wonko the Sane
2
O to chodzi. Koniunktura implikuje, że jest bardzo prawdopodobne, że robi więcej niż jedną rzecz. Na pytanie to tylko coś, co podnosi moją świadomość, że coś MOŻE być nie tak.
JohnFx,
3
Co teraz, jeśli musisz dodać pracownika i zaktualizować jego wypłatę w kilku miejscach? Jeśli ta metoda zawiera dwa wywołania metody ( addEmployee(); updatePayrate();), to nie sądzę, że to problem.
Matt Grande
13

Łą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.

Bob Murphy
źródło
6
Podczas gdy twój punkt jest doskonały, twój ton jest niepotrzebny.
JBRWilkinson
@JBRWilkinson: Dzięki, masz rację. Przepraszam wszystkich.
Bob Murphy
Myślę, że twój nagłówek wymaga ponownego napisania. Zarówno statyczne, jak i dynamiczne łączenie z kodem źródłowym GPL stanowi naruszenie GPL ...
Gavin Coates
Słuszne uwagi. Przepisałem cały post. Dziękuje za wszystko.
Bob Murphy
9

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.
rwong
źródło
18
// TODO: Skomentuj tę odpowiedź
John
8
Myślę, że „język agnostyczny” oznacza teraz „C / C ++ / Java”?
Inaimathi,
+1 „Zgłaszanie wyjątku z niecodziennego powodu” nie może zgodzić się więcej!
billy.bob
2
@Inaimathi - komentarze TODO, kody pośredniczące funkcji, nadużywanie wyjątków, pomieszanie semantyki zerowej / zerowej / pustej i bezcelowe sprawdzanie poprawności są nieodłączne dla wszystkich języków imperatywnych / OOP i do pewnego stopnia wszystkich języków programowania w ogóle.
Rei Miyasaka,
Uważam, że makra preprocesora C małymi literami porządku, ale tylko wtedy, gdy oceniają swoje argumenty tylko raz i dają tylko jedną instrukcję.
Joe D
8

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

Rozpoznać
źródło
2
lub po prostu znak, że są wysoko cenionym indywidualistycznym talentem, który nie został wciągnięty w sieć zhomogenizowanych praktyk kodowania, które nie są w żaden sposób powiązane z „najlepszymi praktykami”.
Anonimowy typ
1
Mam nadzieję, że jesteś sarkastyczny. Jeśli ktoś koduje w tak niecodzienny sposób, to powinno uruchomić dzwonki alarmowe. Mogą być tak artystyczni, jak tylko chcą, ale wciąż ... dzwonki alarmowe.
Ken
2
Istnieje dość nietypowy styl (uważam, że nazywa się to stylem Utrechta), który moim zdaniem jest dość przydatny, mimo że jest niezwykle rzadki poza Haskell / ML / F #. Przewiń w dół do „ Kształtu modułu”: learnyouahaskell.com/making-our-own-types-and-typeclasses . Zaletą tego stylu jest to, że nie trzeba modyfikować separatora w poprzednim wierszu, aby dodać nowy - o czym często zapominam.
Rei Miyasaka,
@ReiMiyasaka Siedem lat spóźnienia, ale ... Utrecht naprawdę mnie denerwuje. Uważam, że błędem w specyfikacji Haskell jest nie narzucanie kolejnej „reguły układu” na listach zorganizowanych pionowo. W ten sposób parser może wykryć nowy wpis listy, po prostu sprawdzając wcięcie, w ten sposób każdy organizuje swoje listy.
Ryan Reich
@RyanReich Weird, siedem lat później i nadal mi się podoba. Zgadzam się jednak; pomimo całej składniowej niezręczności i błędów F # pozwala również na rozdzielanie elementów tylko nową linią i wcięciem (w większości przypadków), co tworzy uporządkowany kod.
Rei Miyasaka,
8

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) { ... }
Yaakov Ellis
źródło
Lub najlepiej: użyj polimorfizmu.
Lstor,
8

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

public DataTable GetEmployees() {...}

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

public void FireEmployee(Object EmployeeObjectOrEmployeeID) {...}
JohnFx
źródło
2
+1: Muszę pracować z niektórymi usługami sieciowymi „REST”, które zwracają wszystko jako tabele pseudo-HTML, nawet jeśli przekazujesz rzeczy, które są wyraźnym błędem składniowym. Nieautoryzowany? Wprowadzić kompletne śmieci? Nadmiar pojemności serwera? 200 (plus wiadomość w okropnym formacie w jednej kolumnie, w jednej tabeli wierszy). AAaaaaaaaargh!
Donal Fellows,
7
  • 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)
Tarka
źródło
7

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.

Vetle
źródło
2
Może to być pedantyczne, ale myślę, że ma znaczenie to, jaką średnią wybierzesz. Jak rozumiem, 50% światowej populacji jest poniżej mediany inteligencji (z definicji); ale inne średnie nie działają w ten sam sposób. Weźmy na przykład populację (1, 1, 1, 5), która ma średnią arytmetyczną 2.
flamingpenguin
ummm, zacytowałeś post „what-superstitions-do-programmers-have”, w którym użytkownik wypowiedział się śmiało o nawiasach klamrowych bez źródła. Nie uważam tego za dobry przykład najlepszych praktyk.
Evan Plaice,
@Evan: tak, masz rację. Dodałem trochę więcej na ten temat, mam nadzieję, że to pomoże.
Vetle
4
Wadą tego są ludzie, którzy niewolniczo przestrzegają „najlepszych praktyk” bez żadnej krytycznej myśli, dlaczego coś jest „najlepszą praktyką”. Właśnie dlatego zdecydowanie nie lubię terminu „najlepsza praktyka”, ponieważ dla niektórych osób jest to wymówka, aby przestać myśleć i podążać za stadem. „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.
Dan Dyer,
Bardzo dobry komentarz, Dan! Do odpowiedzi dodałem dwie ostatnie linie.
Vetle
6

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.

Tim Williscroft
źródło
6

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ą?

Rei Miyasaka
źródło
1
Liczą się schematy XML?
Nick T
1
Pliki konfiguracji systemu. Zazwyczaj powinien być ustawiony przez ./configure, ale nawet to gdzieś potrzebuje wartości domyślnej.
eswald
4
/dev/nulli przyjaciele są w porządku. Ale nawet takie rzeczy /bin/bashsą podejrzane - co, jeśli masz jakiś dziwny system, który ma /usr/bin/bash?
Tom Anderson
1
Kod klienta usługi sieciowej generowany przez narzędzia JAX-WS (przynajmniej JBossWS i Metro) zawiera bezwzględną ścieżkę do pliku WSDL (dwukrotnie!). Co jest prawdopodobnie czymś bardzo niestosownym /home/tom/dev/randomhacking/thing.wsdl. To kryminalne, że jest to zachowanie domyślne.
Tom Anderson
4
about /dev/null: Mam nawyk, gdy programuję w systemie Windows, aby utrzymywać aplikacje i biblioteki c:\dev. Jakoś folder nulljest zawsze automatycznie tworzony w tym folderze. Przysięgam, nie mam pojęcia, kto to robi. (Jedna z moich ulubionych błędów / funkcji)
Sean Patrick Floyd
6

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

Erik van Brakel
źródło
Łapanie ogólnych wyjątków stanowi problem tylko wtedy, gdy rzucisz je bezczynnie. Naprawdę dla większości rzeczy wystarczy jedna klasa wyjątków. Zwykle używam runtime_error.
CashCow
+1 za przykład „wyjątku złap i wyrzuć”. Jeśli nic nie robisz z wyjątkiem, nie łap go. Przynajmniej zaloguj się. Przynajmniej dodaj komentarz wyjaśniający, dlaczego w porządku jest wychwycenie wszystkich wyjątków i przejście do tego momentu w kodzie.
EZ Hart
5

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.

Bryan M.
źródło
5

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.

Mrówka
źródło
5
#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

Sven
źródło
Wczoraj widziałem dokładnie ten „wzorzec” ... w kodzie produkcyjnym ... a nawet gorzej ... w kodzie produkcyjnym C ++: - /
Oliver Weiler