W którym momencie zwięzłość nie jest już cnotą?

Niedawna poprawka wymagała ode mnie przejścia kodu napisanego przez innych członków zespołu, gdzie znalazłem to (to C #):

return (decimal)CostIn > 0 && CostOut > 0 ? (((decimal)CostOut - (decimal)CostIn) / (decimal)CostOut) * 100 : 0;

Teraz, biorąc pod uwagę, że istnieje dobry powód dla wszystkich tych obsad, nadal wydaje się to bardzo trudne do naśladowania. Wystąpił drobny błąd w obliczeniach i musiałem go rozplątać, aby naprawić problem.

Znam styl kodowania tej osoby z przeglądu kodu, a jego podejście jest takie, że krótszy jest prawie zawsze lepszy. I oczywiście jest w tym wartość: wszyscy widzieliśmy niepotrzebnie złożone łańcuchy logiki warunkowej, które można uporządkować za pomocą kilku dobrze umiejscowionych operatorów. Ale jest wyraźnie bardziej biegły ode mnie w śledzeniu łańcuchów operatorów wtłoczonych w jedno stwierdzenie.

Jest to oczywiście kwestia stylu. Ale czy coś zostało napisane lub zbadane na temat rozpoznania punktu, w którym dążenie do zwięzłości kodu przestaje być przydatne i staje się barierą dla zrozumienia?

_{Powodem rzutowania jest Entity Framework. Baza danych musi przechowywać je jako typy zerowalne. Dziesiętny? nie jest równoważny z Decimal w C # i musi zostać obsadzony.}

Aby odpowiedzieć na twoje pytanie dotyczące zachowanych badań

Ale czy coś zostało napisane lub zbadane na temat rozpoznania punktu, w którym dążenie do zwięzłości kodu przestaje być przydatne i staje się barierą dla zrozumienia?

Tak, były prace w tym obszarze.

Aby zrozumieć te rzeczy, musisz znaleźć sposób na obliczenie metryki , aby porównania mogły być dokonywane w oparciu o dane ilościowe (zamiast przeprowadzania porównania w oparciu o dowcip i intuicję, jak robią to inne odpowiedzi). Jednym z potencjalnych wskaźników, które zostały przeanalizowane, jest

Cyklomatyczna złożoność ÷ Źródłowe linie kodu ( SLOC )

W twoim przykładzie ten współczynnik jest bardzo wysoki, ponieważ wszystko zostało skompresowane do jednej linii.

SATC stwierdził, że najskuteczniejszą oceną jest połączenie wielkości i złożoności [Cyclomatic]. Moduły o wysokiej złożoności i dużym rozmiarze mają zwykle najniższą niezawodność. Moduły o niewielkich rozmiarach i dużej złożoności stanowią również ryzyko niezawodności, ponieważ zwykle są bardzo zwięzłym kodem, który trudno jest zmienić lub zmodyfikować.

Połączyć

Oto kilka referencji, jeśli jesteś zainteresowany:

McCabe, T. i A. Watson (1994), Złożoność oprogramowania (CrossTalk: The Journal of Defense Software Engineering).

Watson, AH i McCabe, TJ (1996). Testy strukturalne: metodologia testowania z wykorzystaniem cyklicznej metryki złożoności (publikacja specjalna NIST 500–235). Pobrano 14 maja 2011 r. Ze strony internetowej McCabe Software: http://www.mccabe.com/pdf/mccabe-nist235r.pdf

Rosenberg, L., Hammer, T., Shaw, J. (1998). Metryki oprogramowania i niezawodność (materiały z międzynarodowego sympozjum IEEE na temat inżynierii niezawodności oprogramowania). Pobrano 14 maja 2011 r. Ze strony internetowej Penn State University: http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.104.4041&rep=rep1&type=pdf

Moja opinia i rozwiązanie

Osobiście nigdy nie ceniłem zwięzłości, a jedynie czytelność. Czasami zwięzłość poprawia czytelność, a czasem nie. Co ważniejsze, piszesz kod Rally (Really Obvious Code) zamiast kodu tylko do zapisu (WOC).

Dla zabawy, oto jak bym to napisał i poproś członków mojego zespołu o napisanie:

if ((costIn <= 0) || (costOut <= 0)) return 0;
decimal changeAmount = costOut - costIn;
decimal changePercent = changeAmount / costOut * 100;
return changePercent;

Należy również zauważyć, że wprowadzenie zmiennych roboczych ma pozytywny efekt uboczny polegający na wyzwoleniu arytmetyki punktu stałego zamiast arytmetyki liczby całkowitej, więc potrzeba decimalwyeliminowania wszystkich tych rzutów .

Zwięzłość jest dobra, gdy zmniejsza bałagan wokół rzeczy, które się liczą, ale kiedy staje się zwięzła , pakuje zbyt wiele istotnych danych zbyt gęsto, aby można je było łatwo śledzić, wtedy odpowiednie dane stają się same bałaganem i masz problem.

W tym konkretnym przypadku rzutki decimalsą powtarzane w kółko; prawdopodobnie ogólnie lepiej byłoby przepisać go jako coś w stylu:

var decIn = (decimal)CostIn;
var decOut = (decimal)CostOut;
return decIn > 0 && CostOut > 0 ? (decOut - decIn ) / decOut * 100 : 0;
//                  ^ as in the question

Nagle linia zawierająca logikę jest znacznie krótsza i mieści się na jednej linii poziomej, dzięki czemu można zobaczyć wszystko bez konieczności przewijania, a znaczenie jest znacznie bardziej widoczne.

Chociaż nie mogę przytoczyć żadnych szczegółowych badań na ten temat, sugerowałbym, że wszystkie obsady w twoim kodzie naruszają zasadę „Nie powtarzaj się”. Wygląda na to, że Twój kod próbuje przekonwertować costIni costOutwpisać Decimal, a następnie wykonać pewne testy poprawności wyników takich konwersji oraz wykonać dodatkowe operacje na tych przekonwertowanych wartościach, jeśli testy przejdą pomyślnie. W rzeczywistości kod wykonuje jedną z kontroli poprawności nieprzekonwertowanej wartości, co zwiększa prawdopodobieństwo, że costOut może mieć wartość większą niż zero, ale mniejszą niż połowa wielkości najmniejszej niezerowej, która Decimalmoże reprezentować. Kod byłby znacznie wyraźniejszy, gdyby zdefiniował zmienne typu Decimaldo przechowywania przekonwertowanych wartości, a następnie zastosowałby się do nich.

Wydaje się dziwne, że byłbyś bardziej zainteresowany stosunkiem Decimalreprezentacji costIni costOutniż stosunkiem rzeczywistych wartości costIni costOut, chyba że kod będzie również używał reprezentacji dziesiętnych do innych celów. Jeśli kod zamierza dalej wykorzystywać te reprezentacje, byłby to kolejny argument do tworzenia zmiennych w celu przechowywania tych reprezentacji, zamiast ciągłej sekwencji rzutowań w całym kodzie.

Patrzę na ten kod i pytam „jak koszt może wynosić 0 (lub mniej)?”. Co to za szczególny przypadek? Kod powinien być

bool BothCostsAreValidProducts = (CostIn > 0) && (CostOut > 0);
if (!BothCostsAreValidProducts)
  return NO_PROFIT;
else {
   // that calculation here...
}

Zgaduję, co do nazw tutaj: zmień BothCostsAreValidProductsi NO_PROFITodpowiednio.

Zwięzłość przestaje być cnotą, gdy zapominamy, że jest ona środkiem do celu, a nie cnotą samą w sobie. Lubimy zwięzłość, ponieważ koreluje z prostotą, lubimy prostotę, ponieważ prostszy kod jest łatwiejszy do zrozumienia i łatwiejszy do modyfikacji i zawiera mniej błędów. Na koniec chcemy, aby kod osiągnął ten cel:

1. Spełniaj wymagania biznesowe przy jak najmniejszym nakładzie pracy

2. Unikaj błędów

3. Pozwól nam dokonywać zmian w przyszłości, które nadal spełniają 1 i 2

To są cele. Każda zasada lub metoda projektowania (czy to KISS, YAGNI, TDD, SOLID, proofy, systemy typów, dynamiczne metaprogramowanie itp.) Są uzasadnione tylko w zakresie, w jakim pomagają nam osiągnąć te cele.

Linia, o której mowa, zdaje się nie widzieć celu końcowego. Chociaż jest krótki, nie jest prosty. W rzeczywistości zawiera niepotrzebną redundancję, powtarzając wielokrotnie tę samą operację rzutowania. Powtarzanie kodu zwiększa złożoność i prawdopodobieństwo błędów. Łączenie rzutowania z rzeczywistym obliczeniem utrudnia również przestrzeganie kodu.

Linia ma trzy obawy: Osłony (specjalna obudowa 0), odlewanie typu i obliczenia. Każda sprawa jest dość prosta, gdy rozpatrywana jest w izolacji, ale ponieważ wszystkie zostały zmieszane w tym samym wyrażeniu, trudno jest naśladować.

Nie jest jasne, dlaczego CostOutnie jest obsadzany przy pierwszym użyciu, jest wile CostIn. Może być dobry powód, ale intencja nie jest jasna (przynajmniej nie bez kontekstu), co oznacza, że ​​opiekun będzie ostrożny przed zmianą tego kodu, ponieważ mogą istnieć pewne ukryte założenia. I to jest przekleństwo dla łatwości utrzymania.

Ponieważ CostInjest rzutowany przed porównaniem do 0, zakładam, że jest to wartość zmiennoprzecinkowa. (Gdyby to był int, nie byłoby powodu do rzucania). Ale jeśli CostOutjest liczbą zmiennoprzecinkową, kod może ukryć niejasny błąd dzielenia przez zero, ponieważ wartość zmiennoprzecinkowa może być mała, ale niezerowa, ale zerowa, gdy zostanie ustawiona na dziesiętną (przynajmniej uważam, że to byłoby możliwe).

Tak więc problemem nie jest zwięzłość lub jej brak, problemem jest powtarzająca się logika i splątanie obaw prowadzące do trudnego w utrzymaniu kodu.

Wprowadzenie zmiennych do przechowywania rzutowanych wartości prawdopodobnie zwiększyłoby rozmiar kodu liczonego w liczbie skoków, ale zmniejszyłoby złożoność, oddzielne obawy i poprawiło przejrzystość, co przybliża nas do celu kodu, który jest łatwiejszy do zrozumienia i utrzymania.

Zwięzłość wcale nie jest cnotą. Czytelność jest cnotą.

Zwięzłość może być narzędziem do osiągnięcia cnoty lub, jak w twoim przykładzie, może być narzędziem do osiągnięcia czegoś dokładnie przeciwnego. W ten czy inny sposób nie ma prawie żadnej własnej wartości. Regułę, że kod powinien być „tak krótki, jak to możliwe”, można również zastąpić „tak nieprzyzwoitym, jak to możliwe” - wszystkie są równie bez znaczenia i potencjalnie szkodliwe, jeśli nie służą większemu celowi.

Poza tym opublikowany kod nie jest nawet zgodny z zasadą zwięzłości. Gdyby stałe zostały zadeklarowane za pomocą przyrostka M, (decimal)można byłoby uniknąć większości okropnych rzutów, ponieważ kompilator promowałby pozostanie intdo decimal. Uważam, że osoba, którą opisujesz, używa jedynie zwięzłości jako wymówki. Najprawdopodobniej nie celowo, ale nadal.

W ciągu moich wieloletnich doświadczeń doszedłem do wniosku, że najwyższą zwięzłością jest czas - czas dominuje nad wszystkim innym. Obejmuje to zarówno czas działania - czas potrzebny na wykonanie zadania - jak i czas konserwacji - czas potrzebny na dodanie funkcji lub usunięcie błędów. (Sposób zrównoważenia tych dwóch zależy od tego, jak często wykonywany jest dany kod w porównaniu z ulepszeniami - pamiętaj, że przedwczesna optymalizacja jest nadal źródłem wszelkiego zła .) Zwięzłość kodu ma na celu poprawę zwięzłości obu; krótszy kod zwykle działa szybciej i jest zazwyczaj łatwiejszy do zrozumienia, a zatem utrzymania. Jeśli to nie pomoże, oznacza to ujemny wynik netto.

W przedstawionym tutaj przypadku myślę, że zwięzłość tekstu została źle zinterpretowana jako zwięzłość liczby wierszy, kosztem czytelności, co może wydłużyć czas konserwacji. (Może to również potrwać dłużej, w zależności od sposobu rzutowania, ale jeśli powyższa linia nie zostanie uruchomiona miliony razy, prawdopodobnie nie stanowi to problemu). W takim przypadku powtarzające się rzuty dziesiętne zmniejszają czytelność, ponieważ trudniej jest zobacz, jakie są najważniejsze obliczenia. Napisałbym w następujący sposób:

decimal dIn = (decimal)CostIn;
decimal dOut = (decimal)CostOut;
return dIn > 0 && CostOut > 0 ? ((dOut - dIn) / dOut) * 100 : 0;

(Edycja: jest to ten sam kod, co druga odpowiedź, więc proszę bardzo).

Jestem fanem trójskładnikowego operatora ? :, więc zostawiłbym to.

Podobnie jak prawie wszystkie odpowiedzi powyżej czytelność powinna zawsze być twoim głównym celem. Myślę jednak, że formatowanie może być bardziej efektywnym sposobem na osiągnięcie tego niż tworzenie zmiennych i nowych linii.

return ((decimal)CostIn > 0 && CostOut > 0) ?
       100 * ( (decimal)CostOut - (decimal)CostIn ) / (decimal)CostOut:
       0;

W większości przypadków zdecydowanie zgadzam się z argumentem złożoności cyklicznej, jednak twoja funkcja wydaje się być na tyle mała i prosta, że ​​lepiej poradzić sobie z dobrym przypadkiem testowym. Z ciekawości dlaczego trzeba rzucać na dziesiętne?

Dla mnie wygląda na to, że dużym problemem z czytelnością jest tutaj całkowity brak formatowania.

Chciałbym napisać tak:

return (decimal)CostIn > 0 && CostOut > 0 
            ? (((decimal)CostOut - (decimal)CostIn) / (decimal)CostOut) * 100 
            : 0;

W zależności od tego, czy typ CostIni CostOutjest typem zmiennoprzecinkowym, czy typem integralnym, niektóre rzutowania mogą być również niepotrzebne. W przeciwieństwie do floati double, wartości integralne są domyślnie promowane decimal.

Kod można napisać w pośpiechu, ale powyższy kod powinien być napisany w moim świecie ze znacznie lepszymi nazwami zmiennych.

A jeśli poprawnie odczytam kod, próbuje wykonać obliczenie brutto.

var totalSales = CostOut;
var totalCost = CostIn;
var profit = (decimal)(CostOut - CostIn);
var grossMargin = 0m; //profit expressed as percentage of totalSales

if(profit > 0)
{
    grossMargin = profit/totalSales*100
}

Zakładam, że CostIn * CostOut są liczbami całkowitymi
W ten sposób napisałbym, że
M (Pieniądze) jest dziesiętne

return CostIn > 0 && CostOut > 0 ? 100M * (CostOut - CostIn) / CostOut : 0M;

Kod jest napisany, aby był zrozumiały dla ludzi; zwięzłość w tym przypadku niewiele kupuje i zwiększa obciążenie opiekuna. Z tego powodu należy bezwzględnie rozszerzyć ten kod, czyniąc kod bardziej samowydajnym (lepsze nazwy zmiennych) lub dodając więcej komentarzy wyjaśniających, dlaczego działa w ten sposób.

Kiedy piszesz kod, aby rozwiązać problem dzisiaj, kod ten może być problemem jutro, gdy zmienią się wymagania. Zawsze należy brać pod uwagę utrzymanie, a poprawa zrozumiałości kodu jest niezbędna.

Zwartość nie jest już cnotą, kiedy

• Istnieje podział bez uprzedniej kontroli zera.
• Nie ma możliwości sprawdzenia wartości null.
• Nie ma czyszczenia.
• SPRÓBUJ POŁOWA kontra rzuca łańcuch pokarmowy, w którym można naprawić błąd.
• Przyjmuje się założenia dotyczące kolejności wykonywania zadań asynchronicznych
• Zadania wykorzystujące opóźnienie zamiast zmiany harmonogramu w przyszłości
• Zastosowano niepotrzebne we / wy
• Nie używa aktualizacji optymistycznej

Gdyby to przechodziło testy jednostkowe sprawdzania poprawności, byłoby dobrze, gdyby dodano nową specyfikację, wymagany był nowy test lub ulepszona implementacja i wymagane było „rozplątanie” zwięzłości kodu, to znaczy, gdy pojawiłby się problem.

Oczywiście błąd w kodzie pokazuje, że istnieje inny problem z Q / A, który był niedopatrzeniem, więc fakt, że nie został wykryty, jest powodem do niepokoju.

W przypadku wymagań niefunkcjonalnych, takich jak „czytelność” kodu, musi on zostać zdefiniowany przez kierownika ds. Rozwoju i zarządzany przez głównego programistę oraz przestrzegany przez programistów, aby zapewnić prawidłowe implementacje.

Staraj się zapewnić ustandaryzowaną implementację kodu (standardy i konwencje), aby zapewnić „czytelność” i łatwość „konserwacji”. Ale jeśli te atrybuty jakości nie zostaną zdefiniowane i wymuszone, skończy się to kodem jak w powyższym przykładzie.

Jeśli nie lubisz tego rodzaju kodu, spróbuj uzgodnić zespół ze standardami i konwencjami implementacyjnymi i zanotuj go, a następnie wykonuj losowe przeglądy kodu lub kontrole na miejscu, aby sprawdzić zgodność z konwencją.