101

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.

  • 153
    When brevity trumps readability. – Robert Harvey Jan 5 '17 at 16:23
  • 27
    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 5 '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 5 '17 at 18:26
  • 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 5 '17 at 18:29
  • 5
    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. – Solomonoff's Secret Jan 5 '17 at 18:52

14 Answers 14

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.

  • 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 5 '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 5 '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 5 '17 at 18:34
  • 12
    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 5 '17 at 23:23
  • 30
    "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 6 '17 at 3:48
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.

  • 1
    I probably would have gone farther and refactored ((decOut - decIn ) / decOut) * 100 out to another variable. – FrustratedWithFormsDesigner Jan 5 '17 at 16:39
  • 9
    Assembler was much clearer: only one operation per line. Doh! – user251748 Jan 5 '17 at 18:21
  • 2
    @FrustratedWithFormsDesigner I would even go a step further and wrap the conditional check in parentheses. – Chris Cirefice Jan 5 '17 at 18:23
  • 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 6 '17 at 5:22
  • 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 6 '17 at 13:21
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.

0

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
  • When there is not a long enough Answer. – user251748 Jan 6 '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 Jan 6 '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 6 '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 6 '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 9 '17 at 0:50
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.

  • I am sorry to see this was downvoted with no explanation, but it seems to me to be identical to backpackcodes’s answer minus some of his remarks, so I suppose it was justified. – PJTraill Jan 8 '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 8 '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 8 '17 at 20:18
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.

5

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.

  • 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 at 23:44
0

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 7 '17 at 1:00
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;
  • 1
    hoping they're not both negative thought :p – Walfrat Jan 6 '17 at 13:20
  • 2
    Is the divied by zero still there? – danny117 Jan 6 '17 at 17:07
  • @danny117 When brevity yields the wrong answer then it has gone to far. – paparazzo Jan 6 '17 at 17:11
  • Don't want to update the question and make it active. The 100M and 0M force the decimal. I think (CostOut - CostIn) will be performed as integer math and then the difference is cast to decimal. – paparazzo Jan 8 '17 at 19:21
0

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
}
  • 3
    You lost the divide by zero returns zero. – danny117 Jan 6 '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 – Rudolf Olah Jan 7 '17 at 19:26
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.

  • 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 6 '17 at 16:06
  • Your on the right track. – danny117 Jan 6 '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 at 23:41
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?

  • 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. – Matt Thrower Jan 5 '17 at 21:27
  • 2
    @MattThrower You mean decimal, right? double != decimal, there's a big difference. – pinkfloydx33 Jan 6 '17 at 0:16
  • 1
    @pinkfloydx33 yes! Typing on a phone with only half a brain engaged :) – Matt Thrower Jan 6 '17 at 9: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!" – user251748 Jan 6 '17 at 14:19
  • 3
    I don't find this readable at all. – Almo Jan 7 '17 at 14:06
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.

  • 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 5 '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 6 '17 at 0: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 6 '17 at 0:15
  • 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 6 '17 at 15:21
  • 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 9 '17 at 15:28
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.

  • 5
    Ternaries are hard to read, particularly if there are any expressions beyond a single value or variable in the return values. – Almo Jan 5 '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 5 '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 5 '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. – Matt Thrower Jan 6 '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 Jan 6 '17 at 15:37

protected by gnat Jan 6 '17 at 17:31

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.