Magic values - dlaczego nie

YA
  • Rejestracja: dni
  • Ostatnio: dni
  • Postów: 252
1

Zdarzało się, że krytykowano mnie za to, że mam za dużo nienazwanych stałych w kodzie.

Słyszałem takie opinie, że każda liczba oprócz 0 i 1 oraz każdy string powinny być przypisane do zmiennych i używane tylko za pośrednictwem tych zmiennych, natomiast niedopuszczalne jest przekazanie np. literału 7 jako argumentu dla funkcji.

Mam problem z zaakceptowaniem takiego sposobu myślenia...

Problem polega na tym, że w moim rozumieniu napis

Kopiuj
const jakaśNazwa = 7;

JakiśKod(jakaśNazwa);

ma znaczenie mniej więcej takie: „Programisto, możesz dowolnie zmieniać tę wartość w zależności od potrzeb, kod wykorzystujący tę zmienną jest na tyle ogólny, że zadziała sensownie dla każdej wartości. Przypisanie const jakaśNazwa = 7 będzie równie poprawne, co const jakaśNazwa = 1000, co z kolei będzie równie poprawne, co const jakaśNazwa = -5.” Innymi słowy, tego rodzaju przypisania traktuję jako parametr konfiguracyjny programu.

Podczas gdy napis:

Kopiuj
JakiśKod(7);

ma znaczenie mniej więcej takie: „Programisto, dla jakiejś przyczyny tutaj musi być 7 i tylko 7, nie możesz zmieniać tej wartości, dopóki nie zrozumiesz, jak działa i co zakłada JakiśKod() i skąd wartość 7 się wzięła; prawdopodobnie zmiana tej stałej będzie wymagać modyfikacji funkcji JakiśKod.

Na przykładzie. Niedawno na własne potrzeby napisałem krótki programik układający paski komiksowe, po 7 na stronę A4. Dlaczego akurat 7? Ponieważ znając z góry wymiary tych pasków rozrysowałem je sobie na kartce papieru, doszedłem do wniosku, że akurat 7 wygląda ładnie i ustaliłem, na jaki sposób jest sens je rozłożyć.

Napisałem więc funkcję

Kopiuj
Image<Rgba32> UłóżPaskiNaStronie(List<Image<Rgba32>> paski)
{
    // implementacja
}

Co istotne, funkcja zakłada, że lista będzie miała dokładnie 7 pasków. Nie próbowałem jej nawet generalizować na inną liczbę pasków, raz dlatego, że nie mam takiej potrzeby, dwa, bo musiałyby one wtedy być rozmieszczone na stronie zupełnie inaczej, a trudno, żebym ręcznie rozrysowywał sensowne układy dla każdej liczby pasków na stronie od 1 do 1000 albo trenował w tym celu jakiś model sztucznej inteligencji.

(Inna rzecz, że ta funkcja - z tych samych powodów - zakłada także przybliżone wymiary tych pasków, a w szczególności, że pierwszy na liście będzie "niedzielny", a zatem 2x większy)

Kod wołający tę funkcję:

Kopiuj
List<Image<Rgba32>> PodzielNaStrony(IEnumerable<Image<Rgba32>> paski)
{
    return paski.SplitToChunks(7).Select(chunk => UłóżPaskiNaStronie(chunk)).ToList();
}

(gdzie SplitToChunks to prosta funkcyjka napisana przeze mnie, dzieląca listę na podlisty o długości n elementów).

Tragedia! Magic Value! Należy napisać raczej:

Kopiuj
const int PaskówNaStronę = 7;

List<Image<Rgba32>> PodzielNaStrony(IEnumerable<Image<Rgba32>> paski)
{
    return paski.SplitToChunks(PaskówNaStronę).Select(chunk => UłóżPaskiNaStronie(chunk)).ToList();
}

No ale właśnie program źle zadziała, jeśli zrobimy tu const paskówNaStronę = 8;... a przecież napis const paskówNaStronę = 7; aż zaprasza do zmiany tej liczby na inną.

(Z drugiej strony funkcja SplitToChunks(7) już przyjmuje 7 jako parametr a nie ma tej stałej zahardkodowanej, bo ma zadziałać równie poprawnie dla każdej wartości)

Przypuszczam, że tak rozumując, przeczę ogólnie ustalonym konwencjom... Więc w jaki inny sposób zakomunikować, czy jakąś stałą można dowolnie się bawić, czy też jest ona przez coś wymuszona?

GS
  • Rejestracja: dni
  • Ostatnio: dni
  • Postów: 1265
4

skoro to ma być 7 a nie 8 czy 42, to mamy od tego słówko const i wyrażenia stałe.

YA
  • Rejestracja: dni
  • Ostatnio: dni
  • Postów: 252
1
GutekSan napisał(a):

skoro to ma być 7 a nie 8 czy 42, to mamy od tego słówko const i wyrażenia stałe.

Tak. Tyle, że mi nie chodziło o to, że ktoś może sobie mutować tę wartość podczas runtime, tylko o to, że ktoś może sobie zmienić tę stałą w kodzie i skompilować kod z inną wartością.

Poprawiam OP tak, by to było (mam nadzieję) bardziej jasne.

Wibowit
  • Rejestracja: dni
  • Ostatnio: dni
  • Lokalizacja: XML Hills
7
YetAnohterone napisał(a):

Zdarzało się, że krytykowano mnie za to, że mam za dużo nienazwanych stałych w kodzie.

Też nie lubię, gdy jest za dużo nienazwanych stałych w kodzie.

Co do tej 7 z wywodu w pierwszym poście - przy tak nieoczywistej sprawie (zakładam, że takie zdarzają się rzadko) pewnie bym wydzielił stałą i dodał do niej parę słów komentarza. Samodokumentujący się kod ma swoje granice (nie wszystko jest sens zawierać w nazwach zmiennych, metod, klas itd), a kilka słów komentarza nie powinno być jakieś bardzo uciążliwe w utrzymaniu. Komentarzy się najczęściej (w znakomitej większości przypadków) nie pisze, bo ich utrzymanie kosztuje, ale tutaj większym kosztem jest raczej zastanawianie się skąd się ta 7 wzięła, niż zysk z całkowitego pominięcia opisu tej wartości.

IK
  • Rejestracja: dni
  • Ostatnio: dni
7

Jeśli zawsze chcesz dzielić stronę na 7, to funkcja powinna zawierać tę informację sobie i zawsze dzielić na 7, a nie przyjmować jako argument. YAGNI.

Poza tym, to nie jest tak, że używanie literałów zawsze jest złe. Tradycyjnie: to zależy. Załóżmy, że mamy reader CSV, wtedy wywołanie reader.skipRows(3) jest jasne i wiadomo, że 3 oznacza liczbę wierszy do pominięcia. Inaczej sprawa wygląda przy callSomeApi(3). Co to 3 oznacza? Timeout w sekundach? Ilość retry? Stronę? ID encji?

W niektórych językach jest jeszcze wariant pośredni, named parameters. Wtedy callSomeApi(timeout_in_seconds=3) nie pozostawia wątpliwości.

YA
  • Rejestracja: dni
  • Ostatnio: dni
  • Postów: 252
0
iksde napisał(a):

Jeśli zawsze chcesz dzielić stronę na 7, to funkcja powinna zawierać tę informację sobie i zawsze dzielić na 7, a nie przyjmować jako argument. YAGNI.

No dobrze, ale to jedna funkcja powinna podzielić listę komiksów na kawałki po 7, a druga funkcja powinna te 7 kawałków rozłożyć na stronie?

Inaczej nie będzie zachowana zasada, że "Functions should do one thing. They should do it well. They should do it only."

Silv
  • Rejestracja: dni
  • Ostatnio: dni
  • Lokalizacja: Warszawa
1

@YetAnohterone: o jakim języku mówisz? W jakim języku są przykłady w pierwszym poście? Wspomniane zostało const, ale w różnych językach różne rzeczy oznacza…

KamilAdam
  • Rejestracja: dni
  • Ostatnio: dni
  • Lokalizacja: Silesia/Marki
  • Postów: 5550
1
Silv napisał(a):

@YetAnohterone: o jakim języku mówisz? W jakim języku są przykłady w pierwszym poście?

To chyba C#, tylko oni robia nazwy metod od wielkiej litery

Wspomniane zostało const, ale w różnych językach różne rzeczy oznacza…

Racja, np w takiej Javie const nie ma? Jak to wtedy napisać?

stivens
  • Rejestracja: dni
  • Ostatnio: dni
1

ma znaczenie mniej więcej takie: „Programisto, możesz dowolnie zmieniać tę wartość w zależności od potrzeb, kod wykorzystujący tę zmienną jest na tyle ogólny, że zadziała sensownie dla każdej wartości.

To tylko Twoja interpretacja.

Wartosci ktore mozna w miare dowolnie zmieniac to znajduja sie raczej w configu a nie w kodzie.

EDIT: a w siszarpie nie macie named parameters?
No i inna sprawa, ze skoro musi byc 7 no to to wcale nie jest parametr.

YA
  • Rejestracja: dni
  • Ostatnio: dni
  • Postów: 2384
1

Skoro ma działąć z 7 i tylko z 7. To po co Ci taki parametr? W dodatku jeśli go modyfikujesz, to nie działa :D

Nazwij to sobie zgodnie z tym co robi, jeśli rozkłada wg Twojego layoutu, to coś w stylu splitAccordingTo(YetAnohteroneLayout) i layout wie ile pasków może przyjąć, to samo układaczka pasków.
Pytanie czy tego potrzebujesz? Może będziesz chciał układać na formatach A3, B5 ? Może wiesz, że tylko A4 i nigdy więcej nic więcej?

Pixello
  • Rejestracja: dni
  • Ostatnio: dni
  • Lokalizacja: Podkarpacie
  • Postów: 448
1

Powinna być jakaś licencja na zawód programisty, i na nienazywanie stałych powinna być kara miesiąca pozbawienia prawa do wykonywania zawodu.

JU
  • Rejestracja: dni
  • Ostatnio: dni
  • Postów: 5046
0

Tak jak @yarel powiedział, to layout powinien wiedzieć ile pasków może zmieścić.
A co do Twojego twierdzenia, że stała to coś w rodzaju konfiguracji.... Kod wyrazi tysiąc słów:

const double PI = 3.14;

KamilAdam
  • Rejestracja: dni
  • Ostatnio: dni
  • Lokalizacja: Silesia/Marki
  • Postów: 5550
1

Ja bym zmienił nazwy metod i zamiast kodu:

Kopiuj
List<Image<Rgba32>> PodzielNaStrony(IEnumerable<Image<Rgba32>> paski)
{
    return paski.SplitToChunks(7).Select(chunk => UłóżPaskiNaStronie(chunk)).ToList();
}

To bym napisał:

Kopiuj
List<Image<Rgba32>> PodzielNaStronyPo7Pasków(IEnumerable<Image<Rgba32>> paski)
{
    return paski.SplitToChunks(7).Select(chunk => UłóżPaskiNaStroniePo7(chunk)).ToList();
}

I teraz wiadomo skąd wynika to 7

obscurity
  • Rejestracja: dni
  • Ostatnio: dni
2

tak samo programista może zmienić:

Kopiuj
const paskowNaStrone = 7;

na

Kopiuj
const paskowNaStrone = 8;

jak i:

Kopiuj
return paski.SplitToChunks(7);

na

Kopiuj
return paski.SplitToChunks(8);

nie rozumiem Twojej argumentacji. Jedyna różnica jest taka że w drugim przypadku nie będzie wiadomo za bardzo co zostało zmienione (zwłaszcza przy code review) i liczysz na to że nikt tego nie zmieni bo nie będzie wiedział co zmienia.
Powinieneś nadal napisać co to za wartość co najmniej w komentarzu, albo lepiej - wydzielić do stałej. Nadal możesz w komentarzu napisać czemu nie można tego zmieniać:

Kopiuj
const paskowNaStrone = 7; // musi być 7 bo jestem zodiakalnym rakiem

Stałe nie zachęcają do zmiany, nie wiem skąd taka opinia. Ile razy już w kodzie zmieniłeś:

Kopiuj
const pi = 3.1415...

na inną wartość?

Za to parametry funkcji zachęcają do zmiany. Twoja funkcja nie powinna przyjmować tej wartości 7 jako parametr jeśli działa tylko z siódemką. W tym przypadku nie powinna mieć parametru i mieć stałą wewnątrz siebie.

BTW w .NET 6 masz funkcję Chunk, nie musisz pisać własnej

YA
  • Rejestracja: dni
  • Ostatnio: dni
  • Postów: 252
0
obscurity napisał(a):

Za to parametry funkcji zachęcają do zmiany. Twoja funkcja nie powinna przyjmować tej wartości 7 jako parametr jeśli działa tylko z siódemką. W tym przypadku nie powinna mieć parametru i mieć stałą wewnątrz siebie.

yarel napisał(a):

Skoro ma działąć z 7 i tylko z 7. To po co Ci taki parametr? W dodatku jeśli go modyfikujesz, to nie działa :D

stivens napisał(a):

No i inna sprawa, ze skoro musi byc 7 no to to wcale nie jest parametr.

No ale moja funkcja rozkładająca paski na kartce nie przyjmuje siódemki jako parametru, tylko listę siedmiu obrazków. Siódemka jako parametr jest mi potrzebna, by przygotować tę listę siedmiu obrazków dla tej funkcji?

YA
  • Rejestracja: dni
  • Ostatnio: dni
  • Postów: 252
0

A moze powinna przyjmowac N obrazkow i zwracac ceil(N/7) kartek? :)

I inne posty tego rodzaju.

Tak, ale czy to nie będzie osobna funkcja?

W sensie, wtedy będziemy mieć 2 funkcje, jedna przyjmująca N obrazków i zwracająca ceil(N/7) kartek, ale ta funkcja będzie przecież wołać drugą funkcję, mianowicie tą, która rozkłada akurat 7 obrazków na jednej kartce?

Jeśli mamy coś takiego:

Kopiuj
List<Image<Rbga32>> PodzielPaskiNaStrony(IEnumerable<Image<Rgba32>> paski)
{
    return paski.Chunk(7).Select(ch =>
    {
        // tutaj kod
    }).ToList();
}

To ta funkcja nie robi jednej rzeczy, tylko dwie: dzieli listę i buduje stronę. A więc, zgodnie z zasadami czystego kodu, winna być podzielona na dwie funkcje?

stivens
  • Rejestracja: dni
  • Ostatnio: dni
0

paski.Chunk(7).Select(chunk => RozlozNaKartcePo7(chunk)) jest przeciez ok

i tak jak pisalem wyzej, 7 tutaj to nie jest magic variable (bo jest w kontekscie Chunk).

jarekr000000
  • Rejestracja: dni
  • Ostatnio: dni
  • Lokalizacja: U krasnoludów - pod górą
  • Postów: 4712
3

Dla mnie paski.SplitToChunks(7) - jest zupełnie ok. Podejście @KamilAdam najbardziej mi sie podoba.

consty itp. są do konfiguracji :-)

Riddle
  • Rejestracja: dni
  • Ostatnio: dni
  • Postów: 10227
2

Moim zdaniem wydzielanie stałych żeby je nazwać ma wartość raczej niską.

Jeśli ta wartość faktycznie z jakiegoś powodu faktycznie ma jakieś konkretne znaczenie, to możesz to schować w funkcji lub klasie.

Dla mnie użycie literału 7 w kodzie miałoby sens, gdyby np przyszedł requirement że mam parsować specjalne kody produktów w jakimś sklepie, i w tym sklepie byłby kod Laptop Dell XPS ma kod DLXPSOU0SC, i w tym formacie 8smy znak jest jakiegoś rodzaju sumą kontrolną którą muszę zrobić, wtedy użyłbym literału 7 żeby wyciągnąć ósmą literę, i umieściłbym to pewnie w zmiennej controlSum albo w funkcji getControlSum(), i to działałoby jakos productCode[7]. Ale na pewno nie wydzieliłbym consta int PRODUCT_CODE_CHECKSUM_INDEX = 7, albo const CHECKSUM_INDEX = 7. To byłoby moim zdaniem zaśmiecanie kodu.

Riddle
  • Rejestracja: dni
  • Ostatnio: dni
  • Postów: 10227
0

Słyszałem takie opinie, że każda liczba oprócz 0 i 1 oraz każdy string powinny być przypisane do zmiennych i używane tylko za pośrednictwem tych zmiennych, natomiast niedopuszczalne jest

Zupełny bezsens.

YA
  • Rejestracja: dni
  • Ostatnio: dni
  • Postów: 252
0

Re: Że należy użyć RozlozNaKartcePo7(chunk) czy coś w tym stylu

Ma to sens, przyznaję :)

Jedyne, do czego bym się przyczepił, to to, że nazwa funkcji jednak nie określa wszystkich założeń, jakie funkcja czyni odnośnie swych argumentów - zakłada bowiem jeszcze i to, że paski przekazane jej w argumentach będą miały wymiary mniej więcej takie, jakie wymierzyłem dla tego konkretnego komiksu - ale przeciez trudno, by funkcja miała 120-znakową nazwę, to już chyba trzeba udokumentować... tak samo jak to, że zakłada się, że co 7 pasek w tej długiej, niepodzielonej liście jest paskiem niedzielnym, 2x większym.

obscurity
  • Rejestracja: dni
  • Ostatnio: dni
1

moim zdaniem w kodzie

Kopiuj
return paski.SplitToChunks(7).Select(chunk => UłóżPaskiNaStronie(chunk))

problemem nie jest magiczna 7 w SplitToChunks tylko to że UłóżPaskiNaStronie działa tylko z wartością 7 ale nigdzie o tym nie uprzedza. Popraw UłóżPaskiNaStronie (nazwę lub lepiej - zachowanie) i rozwiązujesz oba problemy.

Ewentualnie przynajmniej defensywnie w UłóżPaskiNaStronie sprawdzić poprawność parametru

Kopiuj
Debug.Assert(chunk.Length <= 7, "Musi być 7 bo tak ładnie według mnie wygląda na moim telefonie");
stivens
  • Rejestracja: dni
  • Ostatnio: dni
1

list.take(7), list.chunk(7) - to jest ok
new Box(20, 40, 30, 25, false, true) juz ni cholery nie jest, duzo lepiej czyta sie new Box(width, height, depth, weight, isFragile, isHighPriority) albo new Box(width = 20, height = 40, depth = 30, weight = 25, isFragile = false, isHighPriority = true)


A zatem

Słyszałem takie opinie, że każda liczba oprócz 0 i 1 oraz każdy string powinny być przypisane do zmiennych i używane tylko za pośrednictwem tych zmiennych, natomiast niedopuszczalne jest przekazanie np. literału 7 jako argumentu dla funkcji.

Taka sugestia?/nakaz? jest bez sensu bo nie zwraca uwagi na kontekst.

To ponizej to natomiast tez nie jest prawda.

Problem polega na tym, że w moim rozumieniu napis

Kopiuj
const jakaśNazwa = 7;

JakiśKod(jakaśNazwa);

ma znaczenie mniej więcej takie: „Programisto, możesz dowolnie zmieniać tę wartość w zależności od potrzeb, kod wykorzystujący tę zmienną jest na tyle ogólny, że zadziała sensownie dla każdej wartości. Przypisanie const jakaśNazwa = 7 będzie równie poprawne, co const jakaśNazwa = 1000, co z kolei będzie równie poprawne, co const jakaśNazwa = -5.” Innymi słowy, tego rodzaju przypisania traktuję jako parametr konfiguracyjny programu.

Podczas gdy napis:

Kopiuj
JakiśKod(7);

ma znaczenie mniej więcej takie: „Programisto, dla jakiejś przyczyny tutaj musi być 7 i tylko 7, nie możesz zmieniać tej wartości, dopóki nie zrozumiesz, jak działa i co zakłada JakiśKod() i skąd wartość 7 się wzięła; prawdopodobnie zmiana tej stałej będzie wymagać modyfikacji funkcji JakiśKod.

Manna5
  • Rejestracja: dni
  • Ostatnio: dni
  • Lokalizacja: Kraków
  • Postów: 667
0

każda liczba oprócz 0 i 1 oraz każdy string powinny być przypisane

Akurat 0 i 1 są tymi liczbami, przy których właśnie można użyć stałych false i true, jeżeli używasz ich w takim znaczeniu.

YA
  • Rejestracja: dni
  • Ostatnio: dni
  • Postów: 252
1

Dobra, próba refaktoryzacji na podstawie Waszych wskazówek

Kopiuj
private record TydzieńPasków(Image<Rgba32> Niedzielny, List<Image<Rgba32>> PonDoSob);

public IEnumerable<Image<Rbga32>> PodzielNaStronyPoTydzień(IEnumerable<Image<Rgba32>> paskiDniamiPoKoleiPierwszyNiedzielny) =>
    paskiDniamiPoKoleiPierwszyNiedzielny.Chunk(7)
        .Select(ch => new TydzieńPasków { Niedzielny: ch.First(), PonDoSob: ch.Skip(1).ToList() })
        .Select(tydzień => UłóżTydzieńNaStronie(tydzień));

private Image<Rgba32> UłóżTydzieńNaStronie(TydzieńPasków tydz)
{
    if(
        !ZakładaneWymiaryNiedzielnego(tydz.Niedzielny) ||
        !PonDoSob.All(pasek => ZakładaneWymiaryZwykłego(pasek)
    ) {
        throw new ArgumentException("Złe wymiary pasków.");
    }

    // ...
    // ...
    // ...
}
Azarien
  • Rejestracja: dni
  • Ostatnio: dni
0
stivens napisał(a):

EDIT: a w siszarpie nie macie named parameters?

Oczywiście że są.

Kopiuj
JakisKod(jakasNazwa: 7);
piotrpo
  • Rejestracja: dni
  • Ostatnio: dni
  • Postów: 3303
6

Magic numbers to raczej coś w tym stylu:

Kopiuj
float calc(int mode, float a, float b){
  switch(mode){
  case 1: return a +b;
  case 2: return a*b;
  case 3: return 1-b;
....
  }
}

Z przypisywaniem każdego literału do stałej - moim zdaniem przegięcie do momentu kiedy czytając konkretną linijkę w kodzie wiem co robi jakaś liczba nie widzę problemu. Gorzej jak się trafi coś w stylu:

Kopiuj
crateWindow(300, 600, 12, #FFFFFF);

gdzie trzeba zajrzeć w dokumentację, żeby obczaić, ze chodzi o szerokość, wysokość, jakieś zrąbane flagi bitowe i kolor tła

Charles_Ray
  • Rejestracja: dni
  • Ostatnio: dni
  • Postów: 1912
5

Poziom dyskusji raczej niski. Znowu ze skrajności w skrajność.

Wynoszenie magic numbers do stałych ma charakter głównie dokumentacyjny, poprawiający czytelność kodu. Mówisz „czym jest to 7” - czy to jakiś default, powszechnie znana wartość, wartość zapewniająca jakiś konkretny flow itd. Dodatkowy atut to możliwość reużycia takiej stałej w innym miejscu. Czy trzeba to stosować bezmyślnie zawsze i wszędzie?

Wibowit
  • Rejestracja: dni
  • Ostatnio: dni
  • Lokalizacja: XML Hills
6
Charles_Ray napisał(a):

Poziom dyskusji raczej niski. Znowu ze skrajności w skrajność.

Ludzie lubią proste, żelazne zasady. Np. zadają pytanie: lepiej skręcać w prawo czy w lewo? Większość odpowiada "w lewo", więc pytacz stwierdza, że od teraz zawsze będzie skręcał w lewo.

piotrpo
  • Rejestracja: dni
  • Ostatnio: dni
  • Postów: 3303
0
Charles_Ray napisał(a):

Wynoszenie magic numbers do stałych ma charakter głównie dokumentacyjny, poprawiający czytelność kodu.

Clean code przestał już być argumentem w przypadku części języków. Takie objective c, gdzie musisz np. podać nazwę parametru. Przykład który podałem wyżej wyglądałby tak (na ile nie udało mi się zapomnieć):
[createWindowWith withdt:300 height:600 windowParametersFlags:12 backgroundColor:#ffffff]

Nie wiem po grzyba wyciągać w tym przypadku to do stałych, jeżeli nie potrzebujemy używać ich ponownie. W wielu innych współczesnych językach można podać te nazwy jeżeli chcemy zwiększyć czytelność kodu. Stare przyzwyczajenia i zasady, które są wciąż stosowane bo kiedyś miały sens. Tak jak część programistów C wciąż używa 4 znaków do nazywania zmiennych, bo kiedyś podobno istniał kompilator, który resztę ignorował.

Zarejestruj się i dołącz do największej społeczności programistów w Polsce.

Otrzymaj wsparcie, dziel się wiedzą i rozwijaj swoje umiejętności z najlepszymi.