0

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. :)

2
  • In addition to the given answers: The function has unclear external dependencies which already makes it not really clear. For example: if (_progressStore.level < 5) return null; could be something like: if(hasUserReachedProgressLevelForRewards()) return null; so the amount of responsibilities of this function goes down. (Single responsibility is a term to look up). All 3 pre-checks could then be grouped in a function: isUserElegibleForLoginReward(). Also the name of the function checkLoginDay is unclear: What does it check? Add that to the name. Jan 9 at 6:25
  • 1
    I like option 1 because it clearly relates the code to business-level requirements. The other options just try to compress the code into fewer statements/lines as if that were somehow better. I've written some thoughts on misapplied cleverness. I am not opposed to using fancy language features if they provide a useful abstraction, but good abstractions hide complexity and make the higher-level code more expressive. All examples in this question are on the same level of abstraction.
    – amon
    Jan 9 at 8:04

4 Answers 4

7

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.

6
  • Good answer. However, you wrote "Variant1 is not terrific." - why do you refer here only to Variant1? AFAIKS all four variants have the same contract issue.
    – Doc Brown
    Jan 8 at 20:53
  • 1
    @DocBrown Yeah. Guess I didn't want to keep flogging same horse as I continued to read the next three. I focused on the first because OP held it up as the gold standard, an improvement over the others, and I wanted to show that there's still room to improve.
    – J_H
    Jan 8 at 21:04
  • Thank you for your comment, this is very helpful! They 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? 2. Better naming in a way that promotes more “natural” reading of the code, more like an English text, f.e.renaming the function title or using helper functions like isEligibleCustomer. Is that correct?
    – Sir Falk
    Jan 9 at 9:08
  • 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". While, as already mentioned, your criticism is very much valid, this question was supposed to be more about the style that you should choose generally (expressiveness vs. conciseness). Do you have any thoughts on that?
    – Sir Falk
    Jan 9 at 9:13
  • 1
    ... or this: if(lastLoginDay == lastDay - with an explaining variable leftRewardedDays = lastDay-lastLoginDay; the test If(leftRewardedDays<0) becomes self-explanatory. That should be the way to go to make the comments superfluous.
    – Doc Brown
    Jan 9 at 16:58
3

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.

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.

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

2
  • 1
    Thank you, this is very helpful. I will try to incorporate some of these practices in my code from now on.
    – Sir Falk
    Jan 9 at 20:52
  • Close enough to what I was thinking of writing: all variants are missing function documentation, which informs both the caller of the function and the reader of the function code understanding what the function is supposed to do. Sure, that can be wrong (especially with copy-pasta) but no more so than the code inside the function.
    – jmoreno
    Jan 10 at 1:11
1

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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