2
\$\begingroup\$

Since last year's CppCon, there is an official set of C++ Guidelines. This long list offers rationales for many important coding style questions and is updated constantly.

Should we refer by links to this list when we do reviews of C++ code? If so, how much of the linked content should be in the answer?

Usually, a guideline consist of a number, a descriptive title, a reason, and some examples/counter examples. As seen in this example:

F.1: "Package" meaningful operations as carefully named functions

Reason

Factoring out common code makes code more readable, more likely to be reused, and limit errors from complex code. If something is a well-specified action, separate it out from its surrounding code and give it a name.

Example, don't

void read_and_print(istream& is)    // read and print an int
{
    int x;
    if (is >> x)
        cout << "the int is " << x << '\n';
    else
        cerr << "no int on input\n";
}

Almost everything is wrong with read_and_print. It reads, it writes (to a fixed ostream), it writes error messages (to a fixed ostream), it handles only ints. There is nothing to reuse, logically separate operations are intermingled and local variables are in scope after the end of their logical use. For a tiny example, this looks OK, but if the input operation, the output operation, and the error handling had been more complicated the tangled mess could become hard to understand.

Note

If you write a non-trivial lambda that potentially can be used in more than one place, give it a name by assigning it to a (usually non-local) variable.

Example

sort(a, b, [](T x, T y) { return x.rank() < y.rank() && x.value() < y.value(); });

Naming that lambda breaks up the expression into its logical parts and provides a strong hint to the meaning of the lambda.

auto lessT = [](T x, T y) { return x.rank() < y.rank() && x.value() < y.value(); };

sort(a, b, lessT);
find_if(a, b, lessT);

The shortest code is not always the best for performance or maintainability.

Exception: Loop bodies, including lambdas used as loop bodies, rarely need to be named. However, large loop bodies (e.g., dozens of lines or dozens of pages) can be a problem. The rule Keep functions short implies "Keep loop bodies short." Similarly, lambdas used as callback arguments are sometimes non-trivial, yet unlikely to be re-usable.

Enforcement

  • See Keep functions short
  • Flag identical and very similar lambdas used in different places.

I would propose to copy the number and title to our question and omit reasons and examples.

What do you think?

Example

Just to make my intention more clear, something like this could be in an answer:

In your code you have multiple locations where you calculate the GCD of two numbers. Instead of the ad hoc code, you should "Package" meaningful operations as carefully named functions (F.1):

int gcd(int a, int b) { return b > 0 ? gcd(b, a % b) : a; }

int a = 15, b = 27, gcdAB = gcd(a, b);
int c = 123, d = 42, gcdCD = gcd(c, d);

Or the guideline title can become a linked heading in the answer:

"Package" meaningful operations as carefully named functions (F.1)

In your code you have multiple locations where you calculate the GCD of two numbers. Instead of the ad hoc code, you should write and reuse functions:

int gcd(int a, int b) { return b > 0 ? gcd(b, a % b) : a; }

int a = 15, b = 27, gcdAB = gcd(a, b);
int c = 123, d = 42, gcdCD = gcd(c, d);

Similarly, you could write your own heading but link it to the appropriate guideline.

\$\endgroup\$
  • \$\begingroup\$ There's a good idea hidden in your proposal, but I disagree with the current form. \$\endgroup\$ – Mast Mar 19 '16 at 13:39
  • 3
    \$\begingroup\$ To quote the guidelines: "This document is a very early draft. It is inkorrekt, incompleat, and pµøoorly (sic) formatted." \$\endgroup\$ – Mast Mar 19 '16 at 13:42
  • \$\begingroup\$ @Mast: I think these guidelines will be in flow forever. However, there are many parts which look quite finished and can be used and in any way the document is a great reference to learn good style on your own. \$\endgroup\$ – Nobody Mar 19 '16 at 14:46
  • \$\begingroup\$ I think your examples added at the bottom are perfectly acceptable. The first part of my answer (regarding whether anyone should be forced to using this stuff in their answers) still stands however. \$\endgroup\$ – nhgrif Mar 19 '16 at 15:09
  • \$\begingroup\$ I don't really see a reason to enforce anything here, or even to have any guidelines about it. If you like an answer, upvote, if you don't like it, downvote. \$\endgroup\$ – Simon Forsberg Mar 19 '16 at 17:11
  • 1
    \$\begingroup\$ @SimonForsberg: Actually I never said to enforce anything. The main purpose of this meta post for me was to educate people about the existence of such guidelines. \$\endgroup\$ – Nobody Mar 19 '16 at 18:51
11
\$\begingroup\$

Should we refer by links to this list when we do reviews of C++ code?

It is up to the individual reviewer. If you consider this to be a valuable C++ resource, then absolutely do so. If others do not consider this a valuable resource, I see not reason for them to include it. If others are unaware, obviously they cannot include it. And even if others are aware and consider it a valuable resource, I don't think they should feel forced into including links to it left, right & center (or even referring to it at all).

If so, how much of the linked content should be in the answer?

Enough that your answer makes total & complete sense as if the guideline resource did not exist at all. This is no different than linking to any other external resource. I shouldn't have to follow a link to make sense of your answer.

I would propose to copy the number and title to our question and omit reasons and examples.

I would disagree. Or at least, I would disagree with one possible interpretation of what you've just said.

For example, the following would make for a terrible code review answer:

Your code is in violation of

F.1: "Package" meaningful operations as carefully named functions

This would be equivalent to me writing an answer that said:

You didn't write very readable code.

It's a link I've sprinkled throughout several of my answers, and while I'd argue that what the reader would find inside the link contains lots of valuable information, the link alone is not going to be enough to make a good answer.

Your answer should:

  • point to specific parts of the asker's code
  • explain what the relevant part of C++ guidelines mean
  • explain why the asker's code is in conflict with the guideline
  • provide examples specific to the asker's code of how to do it better
\$\endgroup\$
  • \$\begingroup\$ Maybe that was not clear from my question but the intention is not to make answers a pure list of links to these guidelines. The offending code should definitely be named and probably be rewritten in corrected form. The idea is to add an explanation via that link (and further material) to avoid plastering answers with long winded explanations and to have a credible source for your claims. Furthermore, adding the number of the guideline (which should not change) work around link rot. \$\endgroup\$ – Nobody Mar 19 '16 at 14:45
  • \$\begingroup\$ The number of the guideline should not change in the same way that links should not rot. The guideline could be restructured, completely removed from the Internet, or go away entirely. Answers should stand on their own and links should exist for further reading on the topic. \$\endgroup\$ – nhgrif Mar 19 '16 at 14:47
  • \$\begingroup\$ I thought I read that the reference numbers would remain once they were assigned. Also, seeing how deeply embedded this project is into the C++ community I would state that it will only vanish together with the community. Finally, my intention is exactly this: further reading. (Also read my addition to the question for more clarity on what I mean.) \$\endgroup\$ – Nobody Mar 19 '16 at 15:02
  • 1
    \$\begingroup\$ I'd make the case that Apple's guidelines regarding the development of iOS applications are as solid and closely intertwined as any set of guidelines and development communities could possibly be (if Apple goes away... so does iOS...). Yet I still expect anything that references any Apple guidelines to stand alone as an answer. \$\endgroup\$ – nhgrif Mar 19 '16 at 15:06

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .