103

A recent bug fix required me to go over code written by other team members, where I found this (it's C#):

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

Now, allowing there's a good reason for all those casts, this still seems very difficult to follow. There was a minor bug in the calculation and I had to untangle it to fix the issue.

I know this person's coding style from code review, and his approach is that shorter is almost always better. And of course there's value there: we've all seen unnecessarily complex chains of conditional logic that could be tidied with a few well-placed operators. But he's clearly more adept than me at following chains of operators crammed into a single statement.

This is, of course, ultimately a matter of style. But has anything been written or researched on recognizing the point where striving for code brevity stops being useful and becomes a barrier to comprehension?

The reason for the casts is Entity Framework. The db needs to store these as nullable types. Decimal? is not equivalent to Decimal in C# and needs to be cast.

Deduplicator
  • 8,591
  • 5
  • 31
  • 50
Bob Tway
  • 3,606
  • 3
  • 21
  • 26
  • 153
    When brevity trumps readability. – Robert Harvey Jan 05 '17 at 16:23
  • 1
    It seems like there's some business/accounting rule to return a zero if CostIn is zero or less, but you can't tell from the code (standard profit calc). The divide by zero check makes sense, but isn't very explicit. – JeffO Jan 05 '17 at 16:52
  • 3
    Brevity can be seen by some as superior or clever code. Your code worker probably likes code golfing and is applying it to his/her daily coding. One Line > [n] Lines... – Jon Raynor Jan 05 '17 at 17:18
  • 1
    For me, the epiphany happened when I had to figure out code I knew I had written years ago. If your code isn't maintainable by even your own standards... – Paul Brinkley Jan 05 '17 at 17:30
  • 28
    Looking at your specific example: a cast is either (1) a place where the developer knows more than the compiler and needs to tell the compiler a fact that cannot be deduced, or (2) where some data is being stored in the "wrong" type for the kinds of operations we need to perform on it. Both are strong indicators that something could be refactored. The best solution here is to find a way to write the code with no casts. – Eric Lippert Jan 05 '17 at 18:24
  • 29
    In particular, it seems bizarre that it is necessary to cast CostIn to decimal to compare it to zero, but not CostOut; why is that? What is on earth is the type of CostIn that it can only be compared to zero by casting it to decimal? And why is CostOut not of the same type as CostIn? – Eric Lippert Jan 05 '17 at 18:26
  • 1
    The Norstrilian motto - **"don't be too clever"** - has always served me well. –  Jan 05 '17 at 18:29
  • 12
    Moreover, the logic here may actually be wrong. Suppose `CostOut` is equal to `Double.Epsilon`, and therefore is greater than zero. But `(decimal)CostOut` is in that case zero, and we have a division by zero error. The first step should be to get the code *correct*, which I think it is not. Get it correct, make test cases, and then make it *elegant*. Elegant code and brief code have a lot in common, but sometimes brevity is not the soul of elegance. – Eric Lippert Jan 05 '17 at 18:29
  • 6
    Brevity is always a virtue. But our objective function combines brevity with other virtues. If one can be briefer without harming other virtues, one always should. – Reinstate Monica Jan 05 '17 at 18:52
  • 2
    It appears that the original author was also already confused by the brevity and thus introduced a bug into the code. – jhyot Jan 05 '17 at 19:30
  • 1
    ____ when this ____ – Joshua Jan 06 '17 at 04:55
  • Don't optimize your code for the compiler unless you absolutely must: write it for other humans. The compiler will understand it anyway. – Martin Schröder Jan 06 '17 at 11:28
  • Brevity and readability are (IMHO) somewhat related: they both correlate to the number of distinct elements in the code. Cramming them all on one line does not reduce the number of them, and frequently makes them more obscure. – Jared Smith Jan 06 '17 at 14:56
  • Seems clear to me. If the costs are positive, get the percentage difference between them. I have seen code that tries to break this up and it becomes too wordy. – Price Jones Jan 06 '17 at 17:31
  • I do have to say, there should be a hotkey/function to untangle stuff like that in every IDE and then retangle it so it doesn't take up 12 lines in the end. – HopefullyHelpful Jan 06 '17 at 18:34
  • " R i g h t . N o w ." – keshlam Jan 07 '17 at 01:06
  • @RobertHarvey Your comment is all that needs to be said on the subject, and it is the epitome of brevity. – sdenham Jan 08 '17 at 00:03
  • Recently hear this Brian Kernighan quote: “Everyone knows that debugging is twice as hard as writing a program in the first place. So if you're as clever as you can be when you write it, how will you ever debug it?” – xdhmoore Jan 08 '17 at 05:09
  • 2
    I spend way too much time in the debugger, so I also argue that brevity is no longer a virtue when you can't easily debug the code, so maybe that's just a corollary to readability . Of course, with this provided sample, the correctness of the code can be achieved with unittests, and is easy to see in the debugger. – Chris O Jan 09 '17 at 12:45
  • I think that logic you mentioned in the question is mixing two things up. Code becomes hard to follow not because it's long. The length or brevity of the code doesn't necessarily have anything to do with its readability. They're two separate axes. Strive for readability, not artificial "brevity". – xji Jan 15 '17 at 09:34
  • All this to avoid an if, which would have been easier to understand and modify. – Thorbjørn Ravn Andersen Sep 01 '17 at 08:58

14 Answers14

163

To answer your question about extant research

But has anything been written or researched on recognizing the point where striving for code brevity stops being useful and becomes a barrier to comprehension?

Yes, there has been work in this area.

To get an understanding of this stuff, you have to find a way to compute a metric so that comparisons can be made on a quantitative basis (rather than just performing the comparison based on wit and intuition, as the other answers do). One potential metric that has been looked at is

Cyclomatic Complexity ÷ Source Lines of Code (SLOC)

In your code example, this ratio is very high, because everything has been compressed onto one line.

The SATC has found the most effective evaluation is a combination of size and [Cyclomatic] complexity. The modules with both a high complexity and a large size tend to have the lowest reliability. Modules with low size and high complexity are also a reliability risk because they tend to be very terse code, which is difficult to change or modify.

Link

Here are a few references if you are interested:

McCabe, T. and A. Watson (1994), Software Complexity (CrossTalk: The Journal of Defense Software Engineering).

Watson, A. H., & McCabe, T. J. (1996). Structured Testing: A Testing Methodology Using the Cyclomatic Complexity Metric (NIST Special Publication 500-235). Retrieved May 14, 2011, from McCabe Software web site: http://www.mccabe.com/pdf/mccabe-nist235r.pdf

Rosenberg, L., Hammer, T., Shaw, J. (1998). Software Metrics and Reliability (Proceedings of IEEE International Symposium on Software Reliability Engineering). Retrieved May 14, 2011, from Penn State University web site: http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.104.4041&rep=rep1&type=pdf

My opinion and solution

Personally, I have never valued brevity, only readability. Sometimes brevity helps readibility, sometimes it does not. What is more important is that you are writing Really Obvious Code(ROC) instead of Write-Only Code (WOC).

Just for fun, here's how I would write it, and ask members of my team to write it:

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

Note also the introduction of the working variables has the happy side effect of triggering fixed-point arithmetic instead of integer arithmetic, so the need for all those casts to decimal is eliminated.

John Wu
  • 26,032
  • 10
  • 63
  • 84
  • 4
    I really like the guard clause for the less than zero case. Might be worthy of a comment: how can a cost be less than zero, what special case is that? – user949300 Jan 05 '17 at 17:05
  • 22
    +1. I would probably add something like `if ((costIn < 0) || (costOut < 0)) throw new Exception("costs must not be negative");` – Doc Brown Jan 05 '17 at 18:01
  • 13
    @DocBrown: That's a good suggestion, but I would consider whether the exceptional code path can be exercised by a test. If yes, then write a test case that exercises that code path. If no, then change the whole thing to an Assert. – Eric Lippert Jan 05 '17 at 18:34
  • 3
    Thank you, Mr. Wu, for this excellent answer with an example of how it should be written for legibility. – Almo Jan 05 '17 at 18:39
  • surely by your suggested measure that if statement should be expanded to if.. elseif..else with lots of curly braces and line breaks? – Ewan Jan 05 '17 at 19:45
  • No metric is perfect. Personally I am not afraid to use a lot of whitespace in the code base as long as it doesn't make functions so long that there is a lot of scrolling needed. – John Wu Jan 05 '17 at 20:05
  • 1
    @ Doc Brown There's actually a better way for the guard clause, just introduce a domain-specific `Cost` value object that prevents a cost to be negative rather than dealing with decimal. It would actually reduce it to `return costOut.minus(costIn).percentOf(costOut);` – plalx Jan 05 '17 at 21:22
  • @Doc Brown I like your exception idea in general, but seeing this code I wonder if they are using negative numbers to flag special cases. (Bad idea but might be the case) My answer below tries to be more explicit. (And arguably it should throw an exception instead of returning 0). – user949300 Jan 05 '17 at 21:42
  • I would not throw on a negative cost. More than once I have purchased an item for a negative price. (I want an X. To satisfy the terms of a promotional offer I also get a Y such that X + Y is cheaper than X alone. In this case I would consider Y to have a negative price.) – Loren Pechtel Jan 05 '17 at 23:15
  • 13
    While these are all good points, I believe the scope of this question is ,code style, not logic. My code snip is an effort at functional equivalence from a black box perspective. Throwing an exception is not the same as returning 0, so that solution would not be functionally equivalent. – John Wu Jan 05 '17 at 23:23
  • 2
    Cyclomatic complexity has been shown, on real code, to be VERY highly correlated with raw SLOC. This means that cyclomatic complexity / SLOC will have very little utility. Consider two routines: one with cyclomatic complexity of 30, containing 300 SLOC, and one with cyclomatic complexity of 3, containing 30 SLOC. In both cases, your metric will return 10, but I think we can agree that those two routines are of NOTHING like equal or even comparable complexity. – John R. Strohm Jan 05 '17 at 23:47
  • 2
    Be careful with your terms. They do not have equivalent *complexity* (you just said that one has 30 and one has 3). But the metric does predict the same *reliability*, if you believe the authors of those studies. – John Wu Jan 05 '17 at 23:52
  • 31
    *"Personally, I have* never *valued brevity, only readability. Sometimes brevity helps readibility, sometimes it does not."* **Excellent** point. +1 just for that. I like your code solution, too. – Wildcard Jan 06 '17 at 03:48
  • well if I really want to talk about the best reading & understanding possible , this `((decOut - decIn ) / decOut) * 100` is way better than the two lines, because I immediatly recognize the simple pattern of a rate computation. While on your two line, it's a bit slower for me. – Walfrat Jan 06 '17 at 13:25
  • 1
    Very nice answer. My only nit to pick is that assigning the return value seems a bit verbose. The method name should take care of that bit of readability, and by making that last bit of logic two lines instead of one, I feel you increase the mental overhead. But, pretty minor nit and you still have my vote. – Adrian Larson Jan 06 '17 at 15:52
  • 2
    @Walfrat I'd have gone with `(1 - decIn / decOut) * 100` for the same reason. – Mathieu Guindon Jan 06 '17 at 19:24
  • 1
    Why the extra parentheses? Unless there's some quirk with this language you can just write `if (costIn <= 0 || costOut <= 0)` right? – DisgruntledGoat Jan 06 '17 at 20:41
  • 1
    Just calculated it using the branches in the generated CIL, the original snippet has a metric value of 4, whereas your modification has a metric value of 0.5. – LegionMammal978 Jan 06 '17 at 22:50
  • +1. Woo hoo, Wu! And in particular: *"Personally, I have never valued brevity, only readability."* However, ironically I must point out that your solution should be method-ized. Two symptoms of "brevity": dumping "brief" code into other, inappropriate methods. And dumping "brief" methods into inappropriate classes -> utter failure to apply SRP at all levels of the code. – radarbob Jan 08 '17 at 20:44
  • You very much changed the semantics, and if we believe OP that the original code contained a bug, then yours is much worse. I'd assume that CostIn, CostOut are not of type Decimal, but for example double, so you duplicate the bug that CostOut > 0 and (Decimal)CostOut > 0 are not equivalent; you calculate CostIn - CostOut in double precision instead of Decimal, changing the result, you perform a division Decimal / double which is the ultimate non-readability problem (because nobody knows the rules for mixed arithmetic), and of course there's the risk of division by zero. – gnasher729 Jun 15 '19 at 23:35
48

Brevity is good when it reduces clutter around the things that matter, but when it becomes terse, packing too much relevant data too densely to easily follow, then the relevant data becomes clutter itself and you have a problem.

In this particular case, the casts to decimal are being repeated over and over; it would probably be better overall to rewrite it as something like:

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

Suddenly the line containing the logic is a lot shorter and fits on one horizontal line, so you can see everything without having to scroll, and the meaning is much more readily apparent.

Deduplicator
  • 8,591
  • 5
  • 31
  • 50
Mason Wheeler
  • 82,151
  • 24
  • 234
  • 309
  • 1
    I probably would have gone farther and refactored `((decOut - decIn ) / decOut) * 100` out to another variable. – FrustratedWithFormsDesigner Jan 05 '17 at 16:39
  • 9
    Assembler was much clearer: only one operation per line. *Doh!* –  Jan 05 '17 at 18:21
  • 2
    @FrustratedWithFormsDesigner I would even go a step further and wrap the conditional check in parentheses. – Chris Cirefice Jan 05 '17 at 18:23
  • @nocomprende there're different assemblers: ARB fragment/vertex programs, for example, can be written in two lines: first line the header like `!!ARBfp1.0`, the second one the whole program. Instructions are separated by semicolons there, and newlines are optional. – Ruslan Jan 05 '17 at 21:36
  • I would even suggest evaluating and storing the conditional. It not only increases readability but is arguably more flexible. Following, `return costPositive ? (decOut - decIn) / decOut * 100 : 0;` is a good compromise of both ideologies. – Legato Jan 05 '17 at 21:46
  • 1
    @FrustratedWithFormsDesigner: Extracting this to before the conditional would bypass the divison-by-zero check (`CostOut > 0`), so you'd have to expand the conditional into an `if`-statement. Not that there's anything wrong with this, but it does add more verbosity than just the introduction of a local. – wchargin Jan 06 '17 at 05:22
  • @wchargin ...which is why I didn't do it in my version. – Mason Wheeler Jan 06 '17 at 10:55
  • 1
    TO be honest, just getting rid of the casts seems is like you did air enough IMO, if someone find hard to read because he can't recognize a simple basic rate computation, that's his problem, not mine. – Walfrat Jan 06 '17 at 13:21
  • 1
    "Brevity is good when it reduces clutter around the things that matter" I really like this statement. – Oliver Jan 09 '17 at 10:55
7

While I can't cite any particular research on the subject, I would suggest that all those casts within your code violate the Don't Repeat Yourself principle. What your code appears to be trying to do is convert the costIn and costOut to type Decimal, and then perform some sanity checks on the results of such conversions, and the performing additional operations on those converted values if the checks pass. In fact, your code performs one of the sanity checks on an unconverted value, raising the possibility that costOut might hold a value which is greater than zero, but less than half the size of than the smallest non-zero that Decimal can represent. The code would be much clearer if it defined variables of type Decimal to hold the converted values, and then acted upon those.

It does seem curious that you would be more interested in the ratio of the Decimal representations of costIn and costOut than the ratio of the actual values of costIn and costOut, unless the code is also going to use the decimal representations for some other purpose. If code is going to make further use of those representations, that would be further argument for creating variables to hold those representations, rather than having a continuing sequence of casts throughout the code.

supercat
  • 8,335
  • 22
  • 28
  • The (decimal) casts are probably to satisfy some business rule requirement. When dealing with accountants you sometimes have to jump through stupid hoops. I thought the CFO was going to have a heart attack when he found the "Use tax roundoff correction -$0.01" line that was unavoidable given the requested functionality. (Supplied: After-tax price. I have to figure the before-tax price. The odds there will be no answer is equal to the tax rate.) – Loren Pechtel Jan 05 '17 at 23:23
  • @LorenPechtel: Given that the precision to which a `Decimal` cast will round depends upon the magnitude of the value in question, I find it hard to imagine business rules which would call for the way the casts are actually going to behave. – supercat Jan 06 '17 at 00:09
  • 1
    Good point--I had forgotten the details of the decimal type because I've never had an occasion to want something like it. I now think this is cargo cult obedience to business rules--specifically, money must not be floating point. – Loren Pechtel Jan 06 '17 at 00:15
  • @LorenPechtel: Unfortunately, Decimal *is* a floating-point type, albeit a rather clumsy and inefficient one. IMHO, a 16-byte fixed-point type would have been better in just about every way. – supercat Jan 06 '17 at 00:29
  • Argh, misread the docs. I was thinking it was fixed point type with decimals. It's something I've never had any occasion to use--I avoid any sort of floating point whenever possible (and by proper choice of units it almost always can be avoided) and I have yet to have any occasion to look beyond 64 bits for integer types. – Loren Pechtel Jan 06 '17 at 02:40
  • The `Decimal` type takes up 16 bytes, which would be more than enough for a fixed-point type with 18 digits to the right of the decimal point and 20 to the left. Decimal is a floating-point type which has about 27 digits of precision, which means for values below a billion it has more precision than a fixed point type, but having absolute precision be fixed is often more important than its exact magnitude, since it makes addition and subtraction transitive. – supercat Jan 06 '17 at 06:03
  • 1
    @LorenPechtel: To clarify my last point: a fixed-point type can guarantee that x+y-y will either overflow or yield y. The `Decimal` type doesn't. The value 1.0d/3.0 will have more digits to the right of the decimal than can be maintained when using larger numbers. So adding and then subtracting the same larger number will cause precision loss. Fixed-point types may lose precision with fractional multiplication or division, but not with addition, subtraction, multiplication, or divide-with-remainder (e.g. 1.00/7 is 0.14 remainder 0.2; 1.00 div 0.15 is 6 remainder 0.10). – supercat Jan 06 '17 at 15:21
  • @supercat "x+y-y will either overflow or yield y" - did you mean "yield x"? – Hulk Jan 09 '17 at 10:26
  • 1
    @Hulk: Yes, of course. I was debating whether to use x+y-y yielding x, x+y-x yielding y, or x+y-x-y, and ended up mish-moshing the first two. What's important is that fixed-point types can guarantee that many sequences of operations will never cause undetected rounding errors of *any* magnitude. If code adds things together in different ways to verify that totals match (e.g. comparing the sum of row subtotals to the sum of column subtotals), having results come out precisely equal is much better than having them come out "close". – supercat Jan 09 '17 at 15:28
6

Brevity stops being a virtue when we forget it is means to an end rather than a virtue in itself. We like brevity because it correlates with simplicity, and we like simplicity because simpler code is easier to understand and easier to modify and contain fewer bugs. In the end we want the code to achieve these goal:

  1. Fulfill the business requirements with the least amount of work

  2. Avoid bugs

  3. Allow us to make changes in the future which continue to fulfill 1 and 2

These are the goals. Any design principle or method (be it KISS, YAGNI, TDD, SOLID, proofs, type systems, dynamic metaprogramming etc.) are only virtuous to the extent they help us achieve these goals.

The line in question seem to have missed sight of the end goal. While it is short, it is not simple. It actually contains needless redundancy by repeating the same casting operation multiple times. Repeating code increases complexity and likelihood of bugs. Intermixing the casting with the actual calculation also makes the code hard to follow.

The line has three concerns: Guards (special casing 0), type casting and calculation. Each concern is pretty simple when taken in isolation, but because it has all been intermingled in the same expression, it becomes hard to follow.

It is not clear why CostOut is not cast the first time it is used wile CostIn is. There might be a good reason, but the intention is not clear (at least not without context) which means a maintainer would be wary of changing this code because there might be some hidden assumptions. And this is anathema to maintainability.

Since CostIn is cast before comparing to 0 I assume it is a floating point value. (If it was an int there would be no reason to cast). But if CostOut is a float then the code might hide an obscure divide-by-zero bug, since a floating point value might be small but non-zero, but zero when cast to a decimal (at least I believe this would be possible.)

So the problem is not brevity or the lack of it, the problem is repeated logic and conflation of concerns leading to hard-to-maintain code.

Introducing variables to hold the casted values would probably increase size of code counted in number of tokes, but will decrease complexity, separate concerns, and improve clarity, which brings us closer to the goal of code which is easier to understand and maintain.

JacquesB
  • 57,310
  • 21
  • 127
  • 176
  • 1
    An important point: Casting CostIn once instead of twice makes it unreadable, because the reader has no idea whether this is a subtle bug with an obvious fix, or whether this is done intentionally. Clearly if I can't say for sure what the writer meant by a sentence, then it isn't readable. There should be either two casts, or a comment explaining why the first use of CostIn doesn't require or shouldn't have a cast. – gnasher729 Jun 15 '19 at 23:44
5

I look at that code and ask "how can a cost be 0 (or less)?". What special case does that indicate? Code should be

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

I'm guessing as to names here: change BothCostsAreValidProducts and NO_PROFIT as appropriate.

Captain Man
  • 586
  • 5
  • 16
user949300
  • 8,679
  • 2
  • 26
  • 35
  • Of course, costs can be zero (think Xmas presents). And in times of negative interest rates negative costs should not be too surprising either and at least the code should be prepared to deal with it (and be it by throwing errors) – Hagen von Eitzen Jan 06 '17 at 16:06
  • Your on the right track. – danny117 Jan 06 '17 at 17:05
  • That's silly. `if (CostIn <= 0 || CostOut <= 0)` is completely fine. – Miles Rout Jan 10 '17 at 20:37
  • Much less readable. The variable name is horrible (BothCostsAreValid would be better. There is nothing here about products). But even that doesn't add anything to the readability, because just checking CostIn, CostOut is fine. You'd introduce an extra variable with a meaningful name if the meaning of the expression that is being tested is not obvious. – gnasher729 Jun 15 '19 at 23:41
3

Brevity is not a virtue at all. Readability is THE virtue.

Brevity can be a tool in achieving the virtue, or, as in your example, can be a tool in achieving something exactly opposite. This way or another, it carries almost no value of it's own. The rule that code should be "as short as possible" can be replaced with "as obscene as possible" just as well - they're all equally meaningless and potentially damaging if they don't serve a greater purpose.

Besides, the code you've posted doesn't even follow rule of brevity. Had the constants be declared with M suffix, most of the horrible (decimal) casts could be avoided, as the compiler would promote remaining int to decimal. I believe that the person you're describing is merely using brevity as an excuse. Most likely not deliberately, but still.

Agent_L
  • 387
  • 1
  • 7
2

In my years of experience, I'm come to believe that the ultimate brevity is that of time - time dominates everything else. That includes both time of performance - how long a program takes to do a job - and time of maintenance - how long it takes to add features or fix bugs. (How you balance those two depends on how often the code in question is being performed vs. improved - remember that premature optimization is still the root of all evil.) Brevity of code is in order to improve brevity of both; shorter code usually runs faster, and is usually easier to understand and therefore maintain. If it doesn't do either, then it's a net negative.

In the case shown here, I think brevity of text has been misinterpreted as brevity of line count, at the expense of readability, which may increase time of maintenance. (It may also take longer to perform, depending on how casting is done, but unless the above line is run millions of times, it's probably not a concern.) In this case, the repetitive decimal casts detract from readability, since it's harder to see what the most important calculation is. I would have written as follows:

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

(Edit: this is the same code as the other answer, so there you go.)

I'm a fan of the ternary operator ? :, so I'd leave that in.

Paul Brinkley
  • 193
  • 7
  • 5
    Ternaries are hard to read, particularly if there are any expressions beyond a single value or variable in the return values. – Almo Jan 05 '17 at 18:40
  • I wonder if that's what's driving the negative votes. Except what I wrote is in much agreement with Mason Wheeler, currently at 10 votes. He left the ternary in, too. I don't know why so many people have a problem with `? :` - I think the example above is sufficiently compact, esp. compared to an if-then-else. – Paul Brinkley Jan 05 '17 at 19:32
  • 1
    Really not sure. I didn't downvote you. I don't like ternaries because it's not clear what's on either side of the `:`. `if-else` reads like english: can't miss what it means. – Almo Jan 05 '17 at 19:37
  • FWIW I suspect you're getting downvotes because this is a very similar answer to Mason Wheeler's but his got their first. – Bob Tway Jan 06 '17 at 13:48
  • 1
    Death to the ternary operator!! (also, death to tabs, nor spaces and any bracketing & indentation model except Allman (in fact, The Donald(tm) has tweeted that these will be the first three laws which he enacts on the 20th) – Mawg says reinstate Monica Jan 06 '17 at 15:37
  • I have but one thing to say to that: cold ? dead : hands – Paul Brinkley Jan 06 '17 at 17:09
  • there's a bug in the code.. that means that no one checked to make sure it worked completely.. that's a bigger problem than how the requirement was implemented – hanzolo Jan 07 '17 at 00:59
  • i'm in agreement about the ternary operator. i don't like it either. in addition, the concept of *"premature optimization"* is debatable. sometimes the correct time to *"optimize"* is immediately when the need and way is apparent. sometimes *"optimization"* is tantamount to correcting a mistake. – robert bristow-johnson Jan 07 '17 at 04:47
2

Like almost all the answers above readability should always be your primary goal. I also however thinking formatting can be a more effective way to achieve this over creating variables and new lines. 

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

I agree strongly with the cyclomatic complexity argument in most cases, however your function appears to be small and simple enough to address better with a good test case. Out of curiosity why the need to cast to decimal?

backpackcoder
  • 181
  • 4
  • 4
    The reason for the casts is Entity Framework. The db needs to store these as nullable types. Double? is not equivalent to Double in C# and needs to be cast. – Bob Tway Jan 05 '17 at 21:27
  • 2
    @MattThrower You mean `decimal`, right? `double` != `decimal`, there's a big difference. – pinkfloydx33 Jan 06 '17 at 00:16
  • 1
    @pinkfloydx33 yes! Typing on a phone with only half a brain engaged :) – Bob Tway Jan 06 '17 at 09:00
  • I have to explain to my students that SQL datatypes are strangely different from the types used in programming languages. I have not been able to explain to them *why* though. "I don't know!" "Little endian!" –  Jan 06 '17 at 14:19
  • 4
    I don't find this readable at all. – Almo Jan 07 '17 at 14:06
  • @Almo: I am surprised by your “not … at all”. Personally I prefer the ‘?’ and ‘:’ at the beginning of the next lines and followed by tabs, to get “syntactic ballast” into predictable positions, but the basic pattern of `_condition_ _when-true_ _when-false_` is sound. But I agree that extracting the `(decimal)` casts and adding comments can be helpful. – PJTraill Jan 08 '17 at 20:16
  • Been programming for decades, and I have never found ternaries readable. Maybe I just have a personal problem. :) – Almo Jan 09 '17 at 02:10
1

Code is written to be understood by people; the brevity in this case doesn't buy much and increases the burden on the maintainer. For this brevity, you absolutely should expand that either by making the code more self-documenting (better variable names) or adding more comments explaining why it works this way.

When you write code to solve a problem today, that code could be a problem tomorrow when requirements change. Maintenance always has to be taken into account and improving understandability of the code is essential.

  • 1
    This is where Software Engineering practices and principles come into play. Non-functional requirements – hanzolo Jan 07 '17 at 01:00
1

To me, it looks as if a big problem with readability here lies in the complete lack of formatting.

I would write it like this:

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

Depending on whether the type of CostIn and CostOut is a floating-point type or an integral type, some of the casts may also be unnecessary. Unlike float and double, integral values are implicitly promoted to decimal.

Felix Dombek
  • 2,109
  • 1
  • 16
  • 24
  • I am sorry to see this was downvoted with no explanation, but it seems to me to be identical to [backpackcodes’s answer](http://softwareengineering.stackexchange.com/a/339517/206403) minus some of his remarks, so I suppose it was justified. – PJTraill Jan 08 '17 at 20:07
  • @PJTraill I must have missed that, it is indeed almost identical. However, I strongly prefer having the operators on the new lines, which is why I'll let my version stand. – Felix Dombek Jan 08 '17 at 20:11
  • I agree about the operators, as I remarked in a comment on the other answer — I hadn’t spotted that you had done it as I prefer. – PJTraill Jan 08 '17 at 20:18
0

I am assuming CostIn * CostOut are integers
This is how I would write it
M (Money) is decimal 

return CostIn > 0 && CostOut > 0 ? 100M * (CostOut - CostIn) / CostOut : 0M;
paparazzo
  • 1,937
  • 1
  • 14
  • 23
0

If this was passing the validation unit tests, then it would be fine, if a new spec was added, a new test or an enhanced implementation was required, and it was required to "untangle" the terseness of the code, that is when the problem would arise.

Obviously a bug in the code shows that there's another issue with Q/A which was an oversight, so the fact that there was a bug that was not caught is cause for concern.

When dealing with non-functional requirements such as "readbility" of code, it needs to be defined by the development manager and managed by the lead developer and respected by the developers to ensure proper implementations.

Try to ensure a standardized implementation of code (standards and conventions) to ensure the "readability" and ease of "maintainability". But if these quality attributes are not defined and enforced, then you will end up with code like the example above.

If you don't like to see this kind of code, then try to get the team in agreement on implementation standards and conventions and write it down and do random code reviews or spot checks to validate the convention is being respected.

hanzolo
  • 3,111
  • 19
  • 21
-1

Brevity is no longer a virtue when

  • There is Division without a prior check for zero.
  • There is no check for null.
  • There is no cleanup.
  • TRY CATCH versus throws up the food chain where the error can be handled.
  • Assumptions are made about the completion order of asynchronous tasks
  • Tasks using delay instead of rescheduling in the future
  • Needless IO is used
  • Not using optimistic update
danny117
  • 130
  • 5
  • When there is not a long enough Answer. –  Jan 06 '17 at 14:21
  • 1
    Ok, this ought to have been a comment, not an answer. But he’s a new guy, so at least explain; don’t just downvote & run away! Welcome aboard, Danny. I cancelled a downvote, but next time make it a comment :-) – Mawg says reinstate Monica Jan 06 '17 at 15:39
  • 2
    Ok I've extended the answer to include some more complex things that I've learned the hard way and the easy way writing brief code. – danny117 Jan 06 '17 at 17:00
  • Thanks for the welcome @Mawg I want to point out the check for null is what I encounter the most causing problems in brief code. – danny117 Jan 06 '17 at 17:09
  • I just edited via Android and it did not ask for a description of the edit. I added optimistic update (detect changed and warn) – danny117 Jan 09 '17 at 00:50
-1

Code can be written in a hurry, but the above code should in my world be written with much better variable names.

And if I read the code correctly then it is trying to make a grossmargin calculation.

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
  • 117
  • 2
  • 3
    You lost the divide by zero returns zero. – danny117 Jan 06 '17 at 17:05
  • 1
    and that's why it's tricky to refactor someone else's code that's maximized for brevity and why it's good to have extra comments to explain why/how things work –  Jan 07 '17 at 19:26