Opublikowano: 06-04-2017



W ramach DSP 2017 w wolnej chwili robię code review. Projektów jest wiele i niemożliwe jest, by przejrzeć cały powstały kod. Zebrałem więc kilka uwag, które powtarzają się najczęściej.

Uwagi są ułożone losowo, nie mają określonej kolejności. Podane przykłady nie odpowiadają kodowi, który był faktycznie komentowany, gdyż są to uwagi ogólne. Zachęcam do podzielenia się własnym doświadczeniem w komentarzu.

Repozytorium (kodu źródłowego?)

Pliki konfiguracyjne IDE nie powinny trafiać do repozytorium razem z kodem - każdy programista może używać innego środowiska. Nawet jeśli każdy w zespole używa tego samego środowiska, to zapewne korzysta z własnej konfiguracji, a ta z repozytorium może ją zmienić.

Repozytorium nie jest także miejscem na pliki binarne, głównie mam na myśli pliki wykonywalne i biblioteki. Nad ich przechowywaniem w repozytorium trzeba się zastanowić i mieć ku temu dobry powód. W większości przypadków nie jest to konieczne, a jedynie zwiększa rozmiar repozytorium. Pliki zostają w nim na zawsze, jeśli takowy zmienimy i dodamy ponownie, to w repozytorium będziemy mieli w całości już dwa takie pliki.

Komentarze w kodzie

Komentarze w kodzie często są zbędne. Niektórzy głoszą nawet pogląd, że w ogóle nie powinno się ich umieszczać. Zobaczmy:

/**
* Created by Adam on 2017-04-06.
*/
class Example ...

Czy ten wygenerowany przez IDE komentarz wnosi coś istotnego? Raczej nie, nigdy też nie zwracamy na niego uwagi. To kiedy plik został utworzony i przez kogo możemy łatwo sprawdzić w systemie kontroli wersji. Może warto wyłączyć generowanie komentarzy przez IDE?

Jednym z ciekawszych miejsc z komentarzami jest ten wątek na Stack Overflow.

Podawane są tam rzeczy ciekawe:

/**
 * Always returns true.
 */
public boolean isAvailable() {
  return false;
}

jak i śmieszne:

long long ago; /* in a galaxy far far away */

Komentarze w systemie kontroli wersji

Komentarz dotyczący commita - jak napisać ten dobry? Najlepszym źródłem, jakie znalazłem o metodzie pisania treści commita, jest artykuł Chrisa Beams "How to Write a Git Commit Message".

Commity o treści "fixes", czy "." nie mówią zbyt wiele. Podobnie jest w przypadku, gdy nadajemy tą samą wiadomość wielu commitom (szczególnie pod rząd). W takim przypadku po wydaniu polecenia git log zobaczymy historię, w której commity mają ten sam komunikat, a zmiany są zupełnie różne.

W treści warto umieszczać znaczące informacje. Po dobrym komunikacie łatwiej jest dowiedzieć się co zmienia dany commit oraz szybko można odnaleźć poszukiwane zmiany.

Krótki przerywnik obrazkowy

/images/xkcd-goto.png

(źródło: xkcd.com )

Magic numbers

Bardzo często pojawiają się w kodzie liczby, które nie za bardzo wiadomo, co oznaczają. Autor zapewne wie, co dana liczba oznacza, ale czytelnik już niekoniecznie. Zobaczmy:

double potentialEnergy(double mass, double height) {
  return mass * height * 9.81;
}

Czym jest 9.81? O wiele lepiej wygląda to:

static final double GRAVITATIONAL_CONSTANT = 9.81;

double potentialEnergy(double mass, double height) {
  return mass * height * GRAVITATIONAL_CONSTANT;
}

Warto skorzystać z możliwości nadania nazwy. Nie jest to kosztowne, a w razie potrzeby można łatwo wyszukać jej wystąpień w kodzie.

Powyższy przykład pochodzi z Source Making .

Polecenia zewnętrzne

Ciągle żywym tematem jest wywoływanie polecenia systemowego, aby zatrzymać przebieg programu tylko po to, by okno konsoli nie zostało zamknięte po zakończeniu wykonywania. Krótko mówiąc:

system("pause");

Jakie problemy widzę w tej linii?

  • rozwiązanie jest nieprzenośne - w systemach Uniksowych nie ma polecenia pause, w związku z tym zadziała to tylko na Windowsie/DOSie ;)
  • zawiera błąd związany z bezpieczeństwem - w Windowsie polecenie do uruchomienia szukane jest w bieżącym katalogu, a następnie w katalogach znajdujących się w zmiennej PATH. Problem ujawni się, dopiero gdy w bieżącym katalogu umieścimy program o nazwie pause. W momencie wykonania naszego programu właśnie to on zostanie uruchomiony.

Rozwiązaniem tego problemu jest odpowiednie skonfigurowanie środowiska, by nie zamykało okna konsoli. Można również uruchamiać skompilowaną aplikację ręcznie z konsoli.

Złożone instrukcje warunkowe

Wyrażenia w instrukcji warunkowej powinny być proste i krótkie, w szczególności należy unikać w nich instrukcji przypisania:

if ((var1 = other.get()) == true) {
  ...
}

Zdecydowanie czytelniejsze jest:

var1 = other.get();
if (var1 == true) {
  ...
}

Czasami może zajść konieczność wydzielenia warunku do osobnej funkcji:

bool isVisible() {
  is_empty = other.isNotEmpty();
  is_marked = other.isMarkedAsVisible();
  is_ready = other.isReadyToDisplay();

  return !is_empty
         && is_marked
         && is_ready;
}

...

if (isVisible()) {
   ...
}

Wydzielenie warunku do osobnej funkcji nie wpływa na wydajność, a ma ogromne znaczenie dla czytelności.

A jakie są Twoje ulubione zapachy kodu?



Comments powered by Disqus