Czysty kod - Czy powinienem zmienić literał 1 na stały?

14

Aby uniknąć magicznych liczb, często słyszymy, że powinniśmy nadać dosłownemu sensownemu imieniu. Jak na przykład:

//THIS CODE COMES FROM THE CLEAN CODE BOOK
for (int j = 0; j < 34; j++) {
    s += (t[j] * 4) / 5;
}

-------------------- Change to --------------------

int realDaysPerIdealDay = 4;
const int WORK_DAYS_PER_WEEK = 5;
int sum = 0;
for (int j = 0; j < NUMBER_OF_TASKS; j++) {
    int realTaskDays = taskEstimate[j] * realDaysPerIdealDay;
    int realTaskWeeks = (realdays / WORK_DAYS_PER_WEEK);
    sum += realTaskWeeks;
}

Mam taką sztuczną metodę:

Wyjaśnij: Przypuszczam, że mam listę osób do obsłużenia i domyślnie wydajemy 5 USD na zakup tylko jedzenia, ale gdy mamy więcej niż jedną osobę, musimy kupić wodę i jedzenie, musimy wydać więcej pieniędzy, może 6 USD. Zmienię kod, proszę skupić się na dosłownym 1 , moim pytaniu na ten temat.

public int getMoneyByPersons(){
    if(persons.size() == 1){ 
        // TODO - return money for one person
    } else {
        // TODO - calculate and return money for people.
    }

}

Kiedy poprosiłem znajomych o przejrzenie mojego kodu, jeden powiedział, że podanie nazwy wartości 1 dałoby czystszy kod, a drugi powiedział, że nie potrzebujemy tutaj stałej nazwy, ponieważ sama wartość jest znacząca.

Więc moje pytanie brzmi: czy powinienem podać nazwę wartości dosłownej 1? Kiedy wartość jest liczbą magiczną, a kiedy nie? Jak odróżnić kontekst, aby wybrać najlepsze rozwiązanie?

Jacek
źródło
Skąd personspochodzi i co opisuje? Twój kod nie ma żadnych komentarzy, więc trudno zgadnąć, co robi.
Hubert Grzeskowiak
7
Nie robi się to jaśniejsze niż 1. Zmieniłbym jednak „rozmiar” na „liczyć”. I moneyService jest głupie, ponieważ powinno być w stanie zdecydować, ile pieniędzy zwrócić w zależności od kolekcji osób. Przekazywałbym więc osoby do getMoney i pozwoliłbym rozwiązać wszelkie wyjątkowe przypadki.
Martin Maat
4
Możesz także wyodrębnić wyrażenie logiczne z klauzuli if do nowej metody. Może to nazwać IsSinglePerson (). W ten sposób wyodrębnisz trochę więcej niż tylko jedną zmienną, ale również
sprawisz,
2
Być może ta logika lepiej pasowałaby w moneyService.getMoney ()? Czy byłby moment, w którym trzeba zadzwonić do getMoney w przypadku 1 osoby? Ale zgodziłbym się z ogólnym sentymentem, że 1 jest jasne. Liczby magiczne to liczby, w których należy podrapać się w głowę, pytając, jak programista doszedł do tej liczby .. tj.if(getErrorCode().equals(4095)) ...
Neil

Odpowiedzi:

23

Nie. W tym przykładzie 1 ma pełne znaczenie.

Co jednak, jeśli liczba osób.size () wynosi zero? Wydaje się dziwne, że persons.getMoney()działa na 0 i 2, ale nie na 1.

użytkownik949300
źródło
Zgadzam się, że 1 ma znaczenie. Dzięki, zaktualizowałem swoje pytanie. Możesz to zobaczyć jeszcze raz, aby uzyskać mój pomysł.
Jack
3
Fraza, którą słyszałem wiele razy na przestrzeni lat, mówi, że magiczne liczby są dowolnymi stałymi nienazwanymi innymi niż 0 i 1. Chociaż potencjalnie potrafię wymyślić przykłady, w których liczby te powinny być nazwane, dość prostą zasadą jest przestrzeganie 99% czas.
Baldrickk
@Baldrickk Czasami inne nazwy liczbowe również nie powinny być ukryte pod nazwą.
Deduplicator
@Deduplicator przychodzą Ci na myśl jakieś przykłady?
Baldrickk
@Baldrickk See amon .
Deduplicator
18

Dlaczego fragment kodu zawiera tę konkretną wartość literalną?

  • Czy ta wartość ma specjalne znaczenie w dziedzinie problemowej? ?
  • A może ta wartość jest tylko szczegółem implementacji , gdzie ta wartość jest bezpośrednią konsekwencją otaczającego kodu?

Jeśli wartość literału ma znaczenie, które nie jest jasne z kontekstu, wówczas tak, pomocne jest nadanie tej wartości nazwy poprzez stałą lub zmienną. Później, gdy pierwotny kontekst zostanie zapomniany, kod o znaczących nazwach zmiennych będzie łatwiejszy do utrzymania. Pamiętaj, że odbiorcami twojego kodu nie jest przede wszystkim kompilator (kompilator z radością będzie pracować ze strasznym kodem), ale przyszli opiekunowie tego kodu - którzy docenią, jeśli kod jest w pewnym stopniu samo wyjaśniający.

  • W pierwszym przykładzie, znaczenie literały takich jak 34, 4, 5nie wynika z kontekstu. Zamiast tego niektóre z tych wartości mają specjalne znaczenie w Twojej domenie problemowej. Dlatego dobrze było nadać im nazwy.

  • W drugim przykładzie znaczenie literału 1jest bardzo jasne z kontekstu. Wprowadzenie nazwy nie jest pomocne.

W rzeczywistości wprowadzanie nazw oczywistych wartości może być również złe, ponieważ ukrywa rzeczywistą wartość.

  • Może to zasłaniać błędy, jeśli nazwana wartość zostanie zmieniona lub była niepoprawna, zwłaszcza jeśli ta sama zmienna zostanie ponownie użyta w niepowiązanych fragmentach kodu.

  • Fragment kodu może również działać dobrze dla określonej wartości, ale może być niepoprawny w ogólnym przypadku. Wprowadzając niepotrzebną abstrakcję, kod nie jest już oczywiście poprawny.

Nie ma ograniczenia rozmiaru liter „oczywistych”, ponieważ zależy to całkowicie od kontekstu. Np. Literał 1024może być całkowicie oczywisty w kontekście obliczania rozmiaru pliku, literał 31w kontekście funkcji skrótu lub literał padding: 0.5emw kontekście arkusza stylów CSS.

amon
źródło
8

Istnieje kilka problemów z tym fragmentem kodu, który, nawiasem mówiąc, można skrócić w następujący sposób:

public List<Money> getMoneyByPersons() {
    return persons.size() == 1 ?
        moneyService.getMoneyIfHasOnePerson() :
        moneyService.getMoney(); 
}
  1. Nie jest jasne, dlaczego jedna osoba jest przypadkiem szczególnym. Przypuszczam, że istnieje konkretna reguła biznesowa, która mówi, że otrzymywanie pieniędzy od jednej osoby różni się radykalnie od otrzymywania pieniędzy od kilku osób. Muszę jednak zajrzeć do środka getMoneyIfHasOnePersoni getMoneymieć nadzieję, że zrozumiem, dlaczego istnieją różne przypadki.

  2. Nazwa getMoneyIfHasOnePersonnie wygląda dobrze. Po nazwisku spodziewałbym się, że metoda sprawdzi, czy jest jedna osoba, a jeśli tak, to dostanie od niego pieniądze; w przeciwnym razie nic nie rób. Z twojego kodu nie dzieje się tak (lub wykonujesz ten warunek dwa razy).

  3. Czy istnieje powód, aby zwrócić List<Money>raczej kolekcję niż kolekcję?

Wracając do twojego pytania, ponieważ nie jest jasne, dlaczego istnieje specjalne traktowanie dla jednej osoby, cyfrę należy zastąpić stałą, chyba że istnieje inny sposób, aby wyjaśnić reguły. Tutaj jedna nie różni się niczym od żadnej innej magicznej liczby. Możesz mieć reguły biznesowe mówiące, że specjalne traktowanie dotyczy jednej, dwóch lub trzech osób, lub tylko więcej niż dwunastu osób.

Jak odróżnić kontekst, aby wybrać najlepsze rozwiązanie?

Robisz wszystko, co czyni kod bardziej wyraźnym.

Przykład 1

Wyobraź sobie następujący fragment kodu:

if (sequence.size() == 0) {
    return null;
}

return this.processSequence(sequence);

Czy zero tutaj jest magiczną wartością? Kod jest raczej jasny: jeśli w sekwencji nie ma żadnych elementów, nie przetwarzajmy go i zwróć specjalną wartość. Ale ten kod można również przepisać w następujący sposób:

if (sequence.isEmpty()) {
    return null;
}

return this.processSequence(sequence);

Tutaj nie ma już stałej, a kod jest jeszcze wyraźniejszy.

Przykład 2

Weź inny fragment kodu:

const result = Math.round(input * 1000) / 1000;

Nie trzeba wiele czasu, aby zrozumieć, co robi w takich językach, jak JavaScript, których nie ma round(value, precision) przeciążenia.

Teraz, jeśli chcesz wprowadzić stałą, jak by się nazywała? Najbliższy termin, jaki można uzyskać, to Precision. Więc:

const precision = 1000;
const result = Math.round(input * precision) / precision;

Czy poprawia to czytelność? Może być. Tutaj wartość stałej jest raczej ograniczona i możesz zadać sobie pytanie, czy naprawdę musisz przeprowadzić refaktoryzację. Zaletą jest to, że teraz precyzja jest deklarowana tylko raz, więc jeśli się zmieni, nie ryzykujesz popełnienia błędu, takiego jak:

const result = Math.round(input * 100) / 1000;

zmieniając wartość w jednym miejscu i zapominając o zrobieniu tego w drugim.

Przykład 3

Z tych przykładów możesz mieć wrażenie, że liczby należy w każdym przypadku zastąpić stałymi . To nie jest prawda. W niektórych sytuacjach posiadanie stałej nie prowadzi do poprawy kodu.

Weź następujący fragment kodu:

class Point
{
    ...
    public void Reset()
    {
        x, y = (0, 0);
    }
}

Jeśli spróbujesz zamienić zera na zmienną, trudność polegałaby na znalezieniu sensownej nazwy. Jak byś to nazwał? ZeroPosition? Base? Default? Wprowadzenie stałej tutaj nie poprawiłoby w żaden sposób kodu. Wydłużyłoby to nieco czas i tylko tyle.

Takie przypadki są jednak rzadkie. Tak więc za każdym razem, gdy znajdziesz numer w kodzie, postaraj się znaleźć sposób, w jaki kod może być refaktoryzowany. Zadaj sobie pytanie, czy liczba ma znaczenie biznesowe. Jeśli tak, stała jest obowiązkowa. Jeśli nie, jak nazwałbyś numer? Jeśli znajdziesz sensowną nazwę, to świetnie. Jeśli nie, istnieje prawdopodobieństwo, że stała jest niepotrzebna.

Arseni Mourzenko
źródło
Dodałbym 3a: nie używaj słowa pieniądze w nazwach zmiennych, używaj kwoty, salda itp. Zbieranie pieniędzy nie ma sensu, zbieranie kwot lub sald ma znaczenie. (OK, mam niewielką kolekcję zagranicznych pieniędzy (a raczej monet), ale to inna sprawa niezwiązana z programowaniem).
Wygięty
3

Możesz stworzyć funkcję, która pobiera pojedynczy parametr i zwraca czasy cztery podzielone przez pięć, które oferują czysty alias do tego, co robi, wciąż korzystając z pierwszego przykładu.

Po prostu oferuję swoją własną strategię, ale może też się czegoś nauczę.

Myślę o tym.

// Just an example name  
function normalize_task_value(task) {  
    return (task * 4) / 5;  // or to `return (task * 4) / NUMBER_OF_TASKS` but it really matters what logic you want to convey and there are reasons to many ways to make this logical. A comment would be the greatest improvement

}  

// Is it possible to just use tasks.length or something like that?  
// this NUMBER_OF_TASKS is the only one thats actually tricky to   
// understand right now how it plays its role.  

function normalize_all_task_values(tasks, accumulator) {  
    for (int i = 0; i < NUMBER_OF_TASKS; i++) {  
        accumulator += normalize_task_value(tasks[i]);
    }
}  

Przepraszam, jeśli jestem poza bazą, ale jestem tylko programistą JavaScript. Nie jestem pewien, jaki skład to uzasadniłem, chyba nie wszystko musi znajdować się w tablicy lub na liście, ale .length miałoby wiele sensu. A * 4 sprawi, że dobra będzie stała, ponieważ jej pochodzenie jest mgliste.

etisdew
źródło
5
Ale co oznaczają te wartości 4 i 5? Jeśli jedno z nich kiedykolwiek będzie wymagało zmiany, czy byłbyś w stanie znaleźć wszystkie miejsca, w których numer jest używany w podobnym kontekście i odpowiednio je zaktualizować? To jest sedno pierwotnego pytania.
Bart van Ingen Schenau,
Myślę, że pierwszy przykład był dość oczywisty, więc nie mogę na nie odpowiedzieć. Operacja nie powinna ulec zmianie w przyszłości, kiedy będziesz musiał napisać nową, aby osiągnąć to, czego potrzebujesz. Myślę, że komentarze byłyby najbardziej korzystne dla tego celu. To połączenie z jedną linią w moim języku. Jak ważne jest, aby osoba świecka była całkowicie zrozumiała?
etisdew
@BartvanIngenSchenau Cała funkcja ma niepoprawną nazwę. Wolałbym lepszą nazwę funkcji, komentarz ze specyfikacją, co funkcja ma zwrócić, i komentarz, dlaczego * 4/5 to osiąga. Stałe nazwy nie są tak naprawdę potrzebne.
gnasher729,
@ gnasher729: Jeśli stałe występują dokładnie raz w kodzie źródłowym dobrze nazwanej, dobrze udokumentowanej funkcji, może nie być konieczne użycie nazwanej stałej. Gdy tylko stała pojawi się wiele razy z tym samym znaczeniem, nadanie tej stałej nazwy gwarantuje, że nie będziesz musiał dowiedzieć się, czy wszystkie te wystąpienia literału 42 oznaczają to samo, czy nie.
Bart van Ingen Schenau
W sercu tego, jeśli oczekujesz, że może zaistnieć potrzeba zmiany wartości tak. Zmieniłbym to jednak na const, aby reprezentować zmienną. Nie sądzę, że tak jest i powinna to być po prostu zmienna pośrednia mieszcząca się w powyższym zakresie. Nie chcę się wykopać i powiedzieć, aby uczynić wszystko parametrem funkcji, ale jeśli można to zmienić, ale rzadko uczyniłbym to parametrem wewnętrznym dla tej funkcji lub zakresu obiektu / struktury, w którym obecnie pracujesz, aby ta zmiana pozostawiła sam zakres globalny.
etisdew
2

Ten numer 1, czy może to być inny numer? Czy może to być 2 lub 3, czy istnieją logiczne powody, dla których musi to być 1? Jeśli musi to być 1, użycie 1 jest w porządku. W przeciwnym razie możesz zdefiniować stałą. Nie nazywaj tej stałej JEDEN. (Widziałem to zrobione).

60 sekund na minutę - potrzebujesz stałej? Cóż, to jest 60 sekund, a nie 50 lub 70. I każdy o tym wie. Więc to może pozostać liczbą.

Wydrukowano 60 pozycji na stronę - liczba ta mogłaby z łatwością wynosić 59, 55 lub 70. W rzeczywistości, jeśli zmienisz rozmiar czcionki, może ona wynosić 55 lub 70. Tak więc tutaj wymagana jest znacząca stała.

To także kwestia tego, jak jasne jest znaczenie. Jeśli napiszesz „minuty = sekundy / 60”, to jasne. Jeśli napiszesz „x = y / 60”, to nie jest jasne. Gdzieś muszą być jakieś sensowne nazwy.

Jest jedna bezwzględna zasada: nie ma bezwzględnych zasad. Poćwicząc, dowiesz się, kiedy używać liczb, a kiedy używać nazwanych stałych. Nie rób tego, bo tak mówi książka - dopóki nie zrozumiesz, dlaczego tak mówi.

gnasher729
źródło
„60 sekund na minutę ... I wszyscy to wiedzą. Więc to może pozostać liczbą”. - Nieważny dla mnie argument. Co jeśli mam 200 miejsc w kodzie, w których użyto „60”? Jak znaleźć miejsca, w których jest on używany w sekundach do minuty, w porównaniu do innych, być może równie „naturalnych” zastosowań? - W praktyce jednak ja też odważę się korzystać minutes*60, czasem nawet hours*3600wtedy, gdy jest mi to potrzebne, bez deklarowania dodatkowych stałych. Na dniach będę prawdopodobnie napisać d*24*3600lub d*24*60*60ponieważ 86400jest blisko do brzegu, gdzie ktoś nie będzie wiedział, że magiczna liczba od pierwszy rzut oka.
JimmyB,
@ JimmyB Jeśli używasz „60” na 200 miejscach w kodzie, jest bardzo prawdopodobne, że rozwiązaniem jest refaktoryzacja funkcji zamiast wprowadzania stałej.
klutt
0

Widziałem sporo kodu, jak w (zmodyfikowanym) OP w pobieraniu DB. Zapytanie zwraca listę, ale reguły biznesowe mówią, że może być tylko jeden element. A potem oczywiście coś się zmieniło dla „tylko tego jednego przypadku” na listę zawierającą więcej niż jeden element. (Tak, powiedziałem wiele razy. To prawie tak, jakby ... nm)

Zamiast więc tworzyć stałą, stworzyłbym (w metodzie czystego kodu) metodę, aby podać nazwę lub wyjaśnić, co warunek ma wykryć (i opisać sposób jego wykrycia):

public int getMoneyByPersons(){
  if(isSingleDepartmentHead()){ 
    // TODO - return money for one person
  } else {
    // TODO - calculate and return money for people.
  }
}

public boolean isSingleDepartmentHead() {
   return persons.size() == 1;
}
Kristian H.
źródło
Zdecydowanie lepiej ujednolicić obsługę. Lista n elementów może być traktowana równomiernie bez względu na to, czym jest n.
Deduplicator
Widziałem, gdyby nie to, że pojedynczy przypadek miał w rzeczywistości inną (marginalną) wartość niż byłby, gdyby ten sam użytkownik był częścią listy o rozmiarze n. W dobrze wykonanym OO może to nie stanowić problemu, ale w przypadku starszych systemów dobra implementacja OO nie zawsze jest możliwa. Najlepsze, co otrzymaliśmy, to rozsądna fasada OO na logice DAO.
Kristian H