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

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?

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.

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.

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.

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.

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.

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;
}