At what point is brevity no longer a virtue?

Question

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

Answer 1

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.

Answer 2

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.

Answer 3

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.

Answer 4

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.

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

Answer 6

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.

Answer 7

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.

Answer 8

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?

Answer 9

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.

Answer 10

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.

Answer 11

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;

Answer 12

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.

Answer 13

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
}

Answer 14

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