Code quality: expressiveness vs. conciseness

Question

I've been wondering about code expressiveness vs. conciseness lately. What I mean by that is that a lot of modern programming languages offer features to express statements in a very short manner, which is cool, but I feel like it can also make it hard to read sometimes. At the same time, working in a team with others on a code base, I noticed that other people don't like to comment a lot, because they say their code and variables are expressive enough, which also falls into the same category in my opinion.

I've tried to construct an example of what that could look like. (Note: this is Dart code, but I think anybody with a Java background will understand). It is a function that is supposed to determine the day of a login reward. If there are no more rewards, return null. The variants below are functionally equivalent.

// Variant 1

int? checkLoginDay(int lastDay) {
  final (lastLoginTimeStamp, lastLoginDay) = _rewardStore.loginDay;
  final now = DateTime.now();

  //don't show if user is not at least level 5
  if (_progressStore.level < 5) return null;

  // don't show if there are no more rewarded days left
  if (lastLoginDay == lastDay) return null;

  // don't show if not at least one day has passed since last login
  if (!now.isDayAfter(lastLoginTimeStamp)) return null;

  return lastLoginDay + 1;
}

// Variant 2

int? checkLoginDay(int lastDay) {
  final (lastLoginTimeStamp, lastLoginDay) = _rewardStore.loginDay;
  final now = DateTime.now();

  if (_progressStore.level < 5) return null;
  if (lastLoginDay == lastDay) return null;
  if (!now.isDayAfter(lastLoginTimeStamp)) return null;

  return lastLoginDay + 1;
}

// Variant 3

int? checkLoginDay(int lastDay) {
  final (lastLoginTimeStamp, lastLoginDay) = _rewardStore.loginDay;
  final now = DateTime.now();

  if (_progressStore.level < 5 || lastLoginDay == lastDay || !now.isDayAfter(lastLoginTimeStamp)) 
    return null;

  return lastLoginDay + 1;
}


// Variant 4

int? checkLoginDay(int lastDay) {
  final (lastLoginTimeStamp, lastLoginDay) = _rewardStore.loginDay;
  return _progressStore.level < 5 || lastLoginDay == lastDay || !now.isDayAfter(lastLoginTimeStamp)
      ? null
      : lastLoginDay + 1;
}

As you can see, the first variant is most expressive, but the last variant is most conciseness. Which variant would you prefer the most? Which way would you generally try to go in your code base - more expressive or more conciseness?

I personally prefer the first variant, because even if it is a lot longer, I can assure that even a year from now, I know what I did and why I did it without much effort.

I am very curious to hear your opinion on this, especially of those who have worked in the field for a long time. If you want to, you can also leave how long you've worked in the industry, or what some experiences with different styles were. :)

Answer 1

contract

Variant1 is not terrific. It offers poor maintainability, and likely wouldn't get past Code Review unchanged.

int? checkLoginDay(int lastDay) {

We started with a verb, good. But what's the contract? How would I know if it "checked" the login day, when I inspect the null-or-int that comes back? What semantics do we place on lastDay? As caller, how would I know if I'm passing in a sensible argument? For example, maybe -1 is prohibited, but that's hard to tell.

The review context helpfully mentioned

determine the day of a login reward. If there are no more rewards, return null.

But we see no javadoc or comments here, so the poor checkLoginDay identifier is left shouldering the burden of explaining the pre- and post-conditions. It turns out it isn't up to the task.

The signature doesn't mention the _rewardStore and _progressStore global input params.

What are the units on days? Maybe they are Julian days? Is it some integer K × 86400, representing seconds since 1970 epoch? Doesn't dart offer a type to describe the concept? Or perhaps we're stuck with just int.

When I read "check" it sounds like we're evaluating for side effects, such as logging that a check failed or raising an exception. So we might call checkFirstInvariant() and checkSecondInvariant(), expecting them to silenty return because all is well.

consistent language in comments

I have to assume that "check" is drawn from the business vocabulary. Reading a little farther on we see:

  //don't show if user is not at least level 5
  if (_progressStore.level < 5) return null;

So we started out "checking", but now we're maybe "showing", which sounds like a presentation layer responsibility.

Rather than these comments, I would prefer to see the part of a requirements document that talks about eligibility for a reward. With those English rules available, reading the Variant2 code would be straightforward.

post-condition

  return lastLoginDay + 1;

The contract seemed to be that we would "check" a "login day". This seems more like we will "get" a "foo day", where it's not yet clear to me how to describe "foo". It doesn't appear to be a day the user is known to have logged in. Maybe it's a "earliest eligibility" day?

Variant 3

  if (_progressStore.level < 5 || lastLoginDay == lastDay || !now.isDayAfter(lastLoginTimeStamp))

I agree with you that, as written, this is unpleasant to read.

But start a pair of lines with || so each disjunct is on its own line, and then it reads at least as nicely as Variant2. Possible downside is that during debugging it's harder to set breakpoint where you want it, or to insert statements for "print debugging".

long ternary

I agree with you that Variant4 is a non-starter.

---

We're examining "expressive" vs. "concise".

The code in Variant2 is not as expressive as it could be. Consider breaking out as many as three helper functions, with descriptive (expressive!) names, such as isEligibleCustomer. Good names obviate the need for verbose comments.

Does that lead to more concise code, in SLOC? Probably not. But it's easier to read, and easier to believe that what you're reading is true. It also gives you the opportunity to unit test individual helpers, for added confidence that the code really does what it claims.

Answer 2

Certainly variants 1 and 2 are the clearest in my judgment, and correspond to how I would likely write the code.

Whether comments are needed or not tends to be context-specific. It helps explain the code here when it is shown in isolation, but as part of a larger codebase you shouldn't need to comment more than a minority of individual lines.

When there is an alternative between an inline expression syntax and a block-structured syntax, I tend to regard it as poor form to use the expression syntax but then split it over multiple lines. Variant 4 is a particular example of this.

My experience is that it's not uncommon that code either needs to be debugged stepwise, that intermediary results need to be inspected and checked, or that the structure of code needs to be supplemented with additional clauses or otherwise rearranged, and all of these are typically inconvenienced by abusing expressions.

Almost all languages have useful features of syntax that can nevertheless be abused in some way.

I think above all though, unless you're running an actual training course, you have to let individual programmers largely determine their own style in their context.

I pay a fair amount of attention to my own style, and what I've found is that there is scarcely any rule of style that can be applied consistently without there being a pathological case.

Answer 3

Here's a related question pair posed by OP Sir Falk, plus a new issue.

The way I understand this, there are 2 main criticisms:

1. Going into pre- / post-conditions and invariants. Do I understand this correctly, that you would prefer more comments on the function input and output variables, for example acceptable ranges or the nature of variables like the units etc?

First let me quibble with the word "comment". I am ambivalent about comments embedded in the body of a function. They can be helpful, when they explain the "why" rather than the "how". They can also have very little value add, when they say what the code already eloquently said. They can also be strongly unhelpful, when (initially accurate) comments drift out of sync with the evolving code, perhaps not keeping up with negation of a condition, or perhaps omitting an important step for maintaining data integrity.

At the top of each function (or method) I always look for a """docstring""", /** javadoc */, or in this context a dart documentation comment. It describes the contract, the single responsibility that this function promises to accomplish. Now, sometimes the method name makes this obvious and there's no need for a javadoc sentence, as in the case of a java getter. We simply verify that it produces no side effects and move on. But sometimes we need a full English sentence to understand caller's and callee's responsibilities, more text than conveniently fits in an identifier.

... acceptable ranges or the nature of variables like the units ...

Yes. Kind of.

If I see an input parameter with a name like fooLength, that's probably enough. The context, the business domain, will describe a Foo for me. The meaning is obvious: clearly SI units of meter, since there was no fooLengthFurlongs annotation. The range is obvious: non-negative, and never a lot bigger than a typical Foo.

In C if I see a time_t, the meaning is similarly obvious: seconds since the 1970 epoch, expressed in UTC, unless there's annotations related to displaying it with some specified zone offset. So again, I know the meaning of the quantity, I know exactly how to interpret those bits.

When I saw you write now = DateTime.now(), I was similarly certain about how to interpret that many microseconds since the epoch. In contrast, the undocumented .loginDay, plus the lastLoginDay and lastDay integers, left me scratching my head. It might have just been a matter of "missing review context" which a GitHub URL would have cleared up -- poke around in the source to see how this project models the notion of "day", perhaps using one epoch or another, perhaps starting at midnight UTC or perhaps using some local zone offset. What I'm saying is that, as a reviewer, I had no idea what it meant, and as a future maintenance engineer I would have no idea how to diagnose / fix a bug or add a feature involving such days, based on what I had read so far.

Using a type more specific than int would have immediately cleared up how to interpret that quantity.

You essentially asked whether I require that explicit "pre- / post-conditions and invariants" be written down. No. But it wouldn't hurt. For example, an assert fishingLineLength < 1e3 at top of function tells me that a reel of nylon will never hold more than a kilometer of material. In other words, caller did the Wrong Thing, caller violated a pre-condition, by passing in a giant value. I wouldn't need to see a >= 0 assertion, but if someone found that verifying that was useful, great, add the check.

Functions seldom implement post-condition assertions. But as a reader, I always want to understand what promise was made, and I want to be able to verify by reading the source that the promise was carried out. For example, a signature of def sqrt(x: float) -> float: implicitly tells me x shall be non-negative, and the post-condition is that square of return value shall be "pretty close" to the input x. A signature of def logOverdraft(amount): tells me that, by the time we exit, we definitely should have logged that amount somewhere.

Loop variants and invariants are seldom written down, because we deliberately strive to write clear code. When reading strcpy's source, it's pretty clear what the src and dst pointers are doing, no need for a comment. But with "tricky" code that manipulates several relationships, I often find myself wishing the author had written more in the middle of the loop about what specifically the loop always makes true.

2. Better naming in a way that promotes more “natural” reading of the code, more like an English text, for example renaming the function title or using helper functions like isEligibleCustomer. Is that correct?

Yes, that is correct. To repeat Doc Brown's observation, a well-named temp var like rewardedDaysRemaining would obviate the need for a comment that describes the meaning of an intermediate expression. Similarly, pushing a boolean expression down into a predicate helper gives us the opportunity to name the expression, revealing its meaning. It also gives us a place to add a documentation sentence, if warranted, and an opportunity to let an automated test suite exercise its several corner cases.

Suppose I'm lost, I encounter Alice and Bob, and I ask for directions. Alice gives me a sequence of "walk north 100m, then east 200m, then south 50m", interjected by Bob's commentary of "hang a right at the light, then another right at the Dunkin sign." Alice gives the (opaque!) function calls, interspersed with Bob's English sentences. I would rather get them all together in one smoothly flowing narrative. I would like the code to read as a story, rather than the comments reading as a story.

---

The newly surfaced issue is:

Also, comparing the 4 variants, I should have maybe mentioned that there is no documentation except for the code (not my choice), and no requirements document. Requirements are basically done "on the fly".

– Sir Falk

Well, that's trouble.

Code is either correct, or it is not. It conforms to its spec, or it doesn't.

There can be no code defect, no bug report, absent a spec.

Every C program has an implicit spec that it shall not segfault or core dump. A more subtle aspect is it shall free() what it malloc()s. Beyond that, we can say little more about an execution than "it ran!"

In your case, you have non-trivial requirements describing when a customer is eligible for rewards. And those requirements are encoded in the source code. Therefore, when the code runs, it cannot possibly do the wrong thing. It's a tautology. What it does, is correct. It seems unlikely that all stakeholders should be interacting with the code to understand how eligibility was handled last year and how revised rules will be used in the coming years.

Recommend that you write a ReadMe.md, a Confluence wiki page, or some other English language document that stakeholders are better equipped to interact with.

Minimally, you have a need to explain to customers what affects their eligibility. And they won't be reading the dart code.

Answer 4

All four examples are well within the acceptable range. I may have my preference, and you should be able to have your own preference. I would probably write one comment block that states the rules used by the function and not split it up as in your first version. And those rules might be in a header file or wherever people can see them without looking at the source code.