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

102

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.

Bob Tway
źródło
153
Gdy zwięzłość przewyższa czytelność.
Robert Harvey,
27
Patrząc na twój konkretny przykład: rzutowanie to albo (1) miejsce, w którym programista wie więcej niż kompilator i musi poinformować kompilator o fakcie, którego nie można wywnioskować, lub (2) gdzie niektóre dane są przechowywane w „złym” „wpisz rodzaje operacji, które musimy na nim wykonać. Oba są mocnymi wskaźnikami, że coś można zrefaktoryzować. Najlepszym rozwiązaniem jest znalezienie sposobu na napisanie kodu bez rzutowania.
Eric Lippert,
29
W szczególności wydaje się dziwne, że trzeba rzucić CostIn na dziesiętne, aby porównać go do zera, ale nie CostOut; dlaczego? Jaki jest na ziemi rodzaj kosztu, w którym można go porównać tylko do zera, wyrzucając go do postaci dziesiętnej? I dlaczego CostOut nie jest tego samego typu co CostIn?
Eric Lippert,
12
Co więcej, logika tutaj może być błędna. Załóżmy, że CostOutjest równy Double.Epsilon, a zatem większy niż zero. Ale (decimal)CostOutw 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.
Eric Lippert,
5
Zwięzłość jest zawsze zaletą. Ale nasza funkcja celu łączy zwięzłość z innymi cnotami. Jeśli ktoś może być krótszy bez szkody dla innych cnót, zawsze powinien.
Tajny Solomonoffa

Odpowiedzi:

163

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 .

John Wu
źródło
4
Naprawdę podoba mi się klauzula ochronna w przypadku mniej niż zero. Może być wart komentarza: jak koszt może być mniejszy od zera, co to za szczególny przypadek?
user949300,
22
+1. Prawdopodobnie dodałbym coś w styluif ((costIn < 0) || (costOut < 0)) throw new Exception("costs must not be negative");
Doc Brown
13
@DocBrown: To dobra sugestia, ale zastanowiłbym się, czy wyjątkową ścieżkę kodu można wykonać w teście. Jeśli tak, napisz przypadek testowy, który ćwiczy tę ścieżkę kodu. Jeśli nie, zmień całość na Assert.
Eric Lippert,
12
Chociaż są to wszystkie dobre punkty, uważam, że zakres tego pytania dotyczy stylu kodu, a nie logiki. Mój wycinek kodu to próba funkcjonalnej równoważności z perspektywy czarnej skrzynki. Zgłoszenie wyjątku nie jest tym samym, co zwrócenie 0, więc rozwiązanie nie byłoby funkcjonalnie równoważne.
John Wu
30
„Osobiście nigdy nie ceniłem zwięzłości, tylko czytelność. Czasami zwięzłość pomaga czytelności, a czasem nie”. Doskonały punkt +1 za to. Podoba mi się również twoje rozwiązanie kodowe.
Wildcard
49

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.

Mason Wheeler
źródło
1
Prawdopodobnie poszedłbym dalej i przekształcił ((decOut - decIn ) / decOut) * 100się w inną zmienną.
FrustratedWithFormsDesigner
9
Asembler był znacznie wyraźniejszy: tylko jedna operacja na linię. Doh!
2
@FrustratedWithFormsDesigner Chciałbym nawet pójść o krok dalej i owinąć sprawdzanie warunkowe w nawiasach.
Chris Cirefice,
1
@FrustratedWithFormsDesigner: Wyodrębnienie tego do warunku pomija sprawdzenie divison-by-zero check ( CostOut > 0), więc musisz rozwinąć warunek w warunek if-statement. Nie chodzi o to, że jest w tym coś złego, ale dodaje więcej gadatliwości niż tylko wprowadzenie lokalnego.
wchargin
1
Szczerze mówiąc, wydaje się, że po prostu pozbycie się obsady jest tak, jakbyś wyemitował wystarczająco dużo IMO, jeśli ktoś trudno jest odczytać, ponieważ nie może rozpoznać prostej obliczenia stawki podstawowej, to jego problem, a nie mój.
Walfrat
7

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.

supercat
źródło
Rzutowania (dziesiętne) prawdopodobnie spełniają pewne wymagania reguł biznesowych. Kiedy masz do czynienia z księgowymi, czasami musisz skakać przez głupie obręcze. Myślałem, że CFO dostanie ataku serca, gdy znalazł wiersz „Użyj korekty zaokrągleń podatkowych - 0,01 $”, który był nieunikniony, biorąc pod uwagę żądaną funkcjonalność. (Dostarczone: Cena po opodatkowaniu. Muszę
obliczyć
@LorenPechtel: Biorąc pod uwagę, że dokładność, z jaką Decimalobsadza się obsada, zależy od wielkości danej wartości, trudno mi sobie wyobrazić reguły biznesowe, które wymagałyby zachowania aktorów.
supercat
1
Dobra uwaga - zapomniałem szczegółów dziesiętnych, ponieważ nigdy nie miałem okazji chcieć czegoś takiego. Myślę teraz, że jest to kultowe posłuszeństwo wobec reguł biznesowych - w szczególności pieniądze nie mogą być zmiennoprzecinkowe.
Loren Pechtel
1
@LorenPechtel: Aby wyjaśnić mój ostatni punkt: typ punktu stałego może zagwarantować, że x + yy przepełni się lub da y. Ten Decimaltyp 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).
supercat
1
@Hulk: Tak, oczywiście. Zastanawiałem się, czy użyć x + yy dającego x, x + yx dającego y, czy x + yxy, i skończyłem mish-moshing przez pierwsze dwa. Ważne jest to, że typy o stałym punkcie mogą zagwarantować, że wiele sekwencji operacji nigdy nie spowoduje niewykrytych błędów zaokrąglania dowolnej wielkości. Jeśli kod sumuje elementy na różne sposoby, aby sprawdzić, czy sumy są zgodne (np. Porównując sumę sum częściowych z sumą sum częściowych), uzyskanie dokładnie takich samych wyników jest o wiele lepsze niż uzyskanie ich „blisko”.
supercat
5

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.

użytkownik949300
źródło
Oczywiście koszty mogą wynosić zero (pomyśl o prezentach świątecznych). A w czasach ujemnych stóp procentowych ujemne koszty też nie powinny być zbyt zaskakujące, a przynajmniej kod powinien być przygotowany, aby sobie z tym poradzić (i to przez rzucanie błędów)
Hagen von Eitzen
Jesteś na dobrej drodze.
danny117
To głupie. if (CostIn <= 0 || CostOut <= 0)jest całkowicie w porządku.
Miles Rout,
O wiele mniej czytelny. Nazwa zmiennej jest okropna (Lepsza byłaby BothCostsAreValid. Nie ma tu nic o produktach). Ale nawet to nie dodaje niczego do czytelności, ponieważ samo sprawdzenie CostIn, CostOut jest w porządku. Wprowadziłbyś dodatkową zmienną o znaczącej nazwie, jeśli znaczenie testowanego wyrażenia nie jest oczywiste.
gnasher729
5

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.

JacquesB
źródło
1
Ważna uwaga: rzutowanie CostIn raz zamiast dwa razy czyni go nieczytelnym, ponieważ czytelnik nie ma pojęcia, czy jest to subtelny błąd z oczywistą poprawką, czy też robi się to celowo. Oczywiście, jeśli nie mogę powiedzieć na pewno, co pisarz miał na myśli, to nie da się tego odczytać. Powinny być albo dwie obsady, albo komentarz wyjaśniający, dlaczego pierwsze użycie CostIn nie wymaga lub nie powinno mieć obsady.
gnasher729
3

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.

Agent_L
źródło
2

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.

Paul Brinkley
źródło
5
Trójskładniki są trudne do odczytania, szczególnie jeśli w zwracanych wartościach występują wyrażenia przekraczające jedną wartość lub zmienną.
Almo
Zastanawiam się, czy to właśnie powoduje negatywne głosy. Z wyjątkiem tego, co napisałem, jest w dużej mierze zgodne z Masonem Wheelerem, obecnie ma 10 głosów. On też opuścił trójkę. Nie wiem, dlaczego tak wiele osób ma problem ? :- myślę, że powyższy przykład jest wystarczająco zwarty, szczególnie. w porównaniu do „jeśli-to-jeszcze”.
Paul Brinkley,
1
Naprawdę nie jestem pewien. Nie głosowałem cię. Nie lubię trójskładników, ponieważ nie jest jasne, co jest po obu stronach :. if-elseczyta po angielsku: nie możesz przegapić tego, co to znaczy.
Almo
FWIW Podejrzewam, że dostajesz głosy negatywne, ponieważ jest to bardzo podobna odpowiedź do Masona Wheelera, ale on ma ich pierwszy.
Bob Tway
1
Śmierć operatora trójskładnikowego !! (również śmierć tabulatorami, spacje i dowolny model nawiasów i wcięć oprócz Allmana (w rzeczywistości The Donald (tm) napisał na Twitterze, że będą to trzy pierwsze prawa, które
uchwali
2

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?

backpackcoder
źródło
4
Powodem rzutowania jest Entity Framework. Baza danych musi przechowywać je jako typy zerowalne. Podwójnie? nie jest równoważny Double w C # i musi zostać obsadzony.
Bob Tway,
2
@MattThrower Masz na myśli decimal, prawda? double! = decimalistnieje duża różnica.
pinkfloydx33
1
@ pinkfloydx33 tak! Pisanie na telefonie z
włączoną
Muszę wyjaśnić moim uczniom, że typy danych SQL dziwnie różnią się od typów używanych w językach programowania. Jednak nie byłem w stanie wyjaśnić im, dlaczego . "Nie wiem!" „Mały Endian!”
3
W ogóle nie uważam tego za czytelne.
Almo
1

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.

Felix Dombek
źródło
Przykro mi, widzę, że zostało to zanegowane bez żadnego wyjaśnienia, ale wydaje mi się, że jest identyczne z odpowiedzią z kodów plecaków pomniejszoną o niektóre jego uwagi, więc przypuszczam, że było to uzasadnione.
PJTraill
@PJTraill Musiałem to przegapić, to jest rzeczywiście prawie identyczne. Jednak zdecydowanie wolę mieć operatorów na nowych liniach, dlatego pozwolę mojej wersji stać.
Felix Dombek
Zgadzam się z operatorami, jak zauważyłem w komentarzu do drugiej odpowiedzi - nie zauważyłem, że zrobiłeś to tak, jak wolę.
PJTraill
0

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
}
Thomas Koelle
źródło
3
Straciłeś podział przez zero, zwraca zero.
danny117
1
i dlatego trudno jest zmienić kod cudzego kodu, który jest zmaksymalizowany pod kątem zwięzłości i dlatego warto mieć dodatkowe komentarze wyjaśniające, dlaczego / jak to działa
Rudolf Olah
0

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;
paparazzo
źródło
1
mając nadzieję, że nie są to zarówno negatywne myśli: p
Walfrat
2
Czy podzielona przez zero wciąż tam jest?
danny117
@ danny117 Kiedy zwięzłość daje złą odpowiedź, zaszła już daleko.
paparazzo
Nie chcę aktualizować pytania i włączać go. 100M i 0M wymuszają dziesiętne. Myślę, że (CostOut - CostIn) będzie wykonywane jako matematyka na liczbach całkowitych, a następnie różnica zostanie zmieniona na dziesiętną.
paparazzo
0

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.

Rudolf Olah
źródło
1
Tutaj mają zastosowanie praktyki i zasady inżynierii oprogramowania. Wymagania
niefunkcjonalne
0

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
danny117
źródło
Gdy nie ma wystarczająco długiej odpowiedzi.
1
Ok, to powinien być komentarz, a nie odpowiedź. Ale jest nowym facetem, więc przynajmniej wyjaśnij; nie tylko głosuj i uciekaj! Witaj na pokładzie, Danny. Anulowałem głosowanie negatywne, ale następnym razem zrób komentarz :-)
Mawg
2
Ok, rozszerzyłem odpowiedź, aby uwzględnić bardziej złożone rzeczy, których nauczyłem się na własnej skórze i łatwy sposób pisania krótkiego kodu.
danny117
Dziękuję za powitanie @Mawg Chcę zaznaczyć, że sprawdzenie, czy null jest tym, co napotyka najbardziej powodujące problemy w krótkim kodzie.
danny117
Właśnie edytowałem na Androidzie i nie poprosiłem o opis zmiany. Dodałem aktualizację optymistyczną (wykryj zmienioną i ostrzegaj)
danny117
0

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ą.

hanzolo
źródło