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.
źródło
CostOut
jest równyDouble.Epsilon
, a zatem większy niż zero. Ale(decimal)CostOut
w tym przypadku wynosi zero i mamy błąd dzielenia przez zero. Pierwszym krokiem powinno być poprawienie kodu , co moim zdaniem nie jest. Popraw to, twórz przypadki testowe, a następnie spraw, by były eleganckie . Elegancki kod i krótki kod mają ze sobą wiele wspólnego, ale czasem zwięzłość nie jest duszą elegancji.Odpowiedzi:
Aby odpowiedzieć na twoje pytanie dotyczące zachowanych badań
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.
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:
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
decimal
wyeliminowania wszystkich tych rzutów .źródło
if ((costIn < 0) || (costOut < 0)) throw new Exception("costs must not be negative");
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
decimal
są powtarzane w kółko; prawdopodobnie ogólnie lepiej byłoby przepisać go jako coś w stylu: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.
źródło
((decOut - decIn ) / decOut) * 100
się w inną zmienną.CostOut > 0
), więc musisz rozwinąć warunek w warunekif
-statement. Nie chodzi o to, że jest w tym coś złego, ale dodaje więcej gadatliwości niż tylko wprowadzenie lokalnego.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ć
costIn
icostOut
wpisać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óraDecimal
może reprezentować. Kod byłby znacznie wyraźniejszy, gdyby zdefiniował zmienne typuDecimal
do przechowywania przekonwertowanych wartości, a następnie zastosowałby się do nich.Wydaje się dziwne, że byłbyś bardziej zainteresowany stosunkiem
Decimal
reprezentacjicostIn
icostOut
niż stosunkiem rzeczywistych wartościcostIn
icostOut
, 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.źródło
Decimal
obsadza się obsada, zależy od wielkości danej wartości, trudno mi sobie wyobrazić reguły biznesowe, które wymagałyby zachowania aktorów.Decimal
typ nie. Wartość 1,0d / 3,0 będzie miała więcej cyfr po prawej stronie dziesiętnej, niż można zachować przy użyciu większych liczb. Tak więc dodanie, a następnie odjęcie tej samej większej liczby spowoduje utratę precyzji. Typy stałoprzecinkowe mogą tracić precyzję z częściowym pomnożeniem lub dzieleniem, ale nie z dodawaniem, odejmowaniem, mnożeniem lub dzieleniem z resztą (np. 1,00 / 7 to 0,14 reszty 0,2; 1,00 dyw 0,15 to 6 reszty 0,10).Patrzę na ten kod i pytam „jak koszt może wynosić 0 (lub mniej)?”. Co to za szczególny przypadek? Kod powinien być
Zgaduję, co do nazw tutaj: zmień
BothCostsAreValidProducts
iNO_PROFIT
odpowiednio.źródło
if (CostIn <= 0 || CostOut <= 0)
jest całkowicie w porządku.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:
Spełniaj wymagania biznesowe przy jak najmniejszym nakładzie pracy
Unikaj błędów
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
CostOut
nie jest obsadzany przy pierwszym użyciu, jest wileCostIn
. 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ż
CostIn
jest 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śliCostOut
jest 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.
źródło
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 pozostanieint
dodecimal
. Uważam, że osoba, którą opisujesz, używa jedynie zwięzłości jako wymówki. Najprawdopodobniej nie celowo, ale nadal.źródło
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:
(Edycja: jest to ten sam kod, co druga odpowiedź, więc proszę bardzo).
Jestem fanem trójskładnikowego operatora
? :
, więc zostawiłbym to.źródło
? :
- myślę, że powyższy przykład jest wystarczająco zwarty, szczególnie. w porównaniu do „jeśli-to-jeszcze”.:
.if-else
czyta po angielsku: nie możesz przegapić tego, co to znaczy.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.
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?
źródło
decimal
, prawda?double
! =decimal
istnieje duża różnica.Dla mnie wygląda na to, że dużym problemem z czytelnością jest tutaj całkowity brak formatowania.
Chciałbym napisać tak:
W zależności od tego, czy typ
CostIn
iCostOut
jest typem zmiennoprzecinkowym, czy typem integralnym, niektóre rzutowania mogą być również niepotrzebne. W przeciwieństwie dofloat
idouble
, wartości integralne są domyślnie promowanedecimal
.źródło
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.
źródło
Zakładam, że CostIn * CostOut są liczbami całkowitymi
W ten sposób napisałbym, że
M (Pieniądze) jest dziesiętne
źródło
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.
źródło
Zwartość nie jest już cnotą, kiedy
źródło
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ą.
źródło