Define coding style guidelines

[pmconrad]

User Story
As a contributor I want coding style guidelines so that our code becomes more readable through consistent formatting and practices.

Impacts

• Other (please add below)
  Code quality

Additional Context (optional)
https://isocpp.org/wiki/faq/coding-standards

CORE TEAM TASK LIST

• Consensus regarding formatting rules
  • Formatting
    • Maximum line length: 118
    • EOF: 1 newline
    • Code block separation: 1 newline
    • Long calls: Use 496c622
    • Spaces: In discussion. See Wiki
  • Comments
    • In-line
    • In code blocks
    • In test cases
  • Naming
    • Methods: all lowercase with "_" as word separator
    • Other?
  • Scoping
    • In lambda:
    • In large refactor:
    • Avoid global variables
  • Classes
    • Please add
  • Functions
    • Please add
  • Exception Rules
    • Please add
  • Architecture
    • Create/modify/delete of the database inside operation evaluator must be done in do_apply and never in do_evaluate
    • Bump database: when new fields are added or removed to objects
      • DATABASE_VERSION format: YYYYMMDD
  • PR against:
    • develop if no impact to consensus logic
    • hardfork if consensus impacting
• Identify a tool(s) that can do it automatically
• Perform whitespace-only change

[cogutvalera]

As a developer/contributor I want to see too code convention. Maybe some kind of documentation for example as Google C++ Style Guide

https://google.github.io/styleguide/cppguide.html

[oxarbitrage]

i think i can take care of this if you guys think i can have the skills needed. i will mention code styles briefly in bitfest presentation so i am researching a bit in the subject.

[oxarbitrage]

I will try to collect some style guides as they happen, have 2 right now, please feel free to link more:

• Method names should be all lowercase with "_" as word separator: refine es_objects plugin #1271 (comment)
• Long calls, definitions or declarations are recommended to use this style: 496c622

Some basics rules:

• 3 spaces indentation everywhere possible.
• newline at the end of each file.

I think we should tag code style changes commit with something, IIRC @pmconrad wrote a commit names guide for us but i cant find it.

[oxarbitrage]

When defining a function: { should always be in a new line.

What about if or for ? Should we always use it in a new line or there are exceptions ?

Should we use:

if(whatever)
   dowhatever();

or:

if(whatever)
{
   dowhatever();
}

or:

if(whatever) {
   dowhatever();
}

or:

if(whatever) dowhatever();

[pmconrad]

Current style has opening/closing brace always on a line by itself, and braces only where needed.

For single-statement if's, leaving out the braces is dangerous, as in

if( x )
   if( y )
      doA();
else
   doB();

...but due to the brace-in-separate-line rule, adding braces costs a lot of vertical space. Most Java code always has braces even for single-line blocks, with the opening brace on the same line as the if, while or for. I'd prefer that, but our current code doesn't do it anywhere AFAIK.

I don't think I ever wrote a commit-names guide. But whitespace-only changes should come in a separate commit so they can easily be verified with diff -w.

[oxarbitrage]

I don't think I ever wrote a commit-names guide. But whitespace-only changes should come in a separate commit so they can easily be verified with diff -w.

I probably confused myself with this.

Current style has opening/closing brace always on a line by itself, and braces only where needed.

We can stick to this, but if nested blocks, only the most inner one can save the braces.

[jmjatlanta]

* newline at the end of each file.

Pure curiosity... Why?

[abitmore]

Pure curiosity... Why?

Probably due to build scripts like this used in the project:

bitshares-core/libraries/chain/CMakeLists.txt

Lines 2 to 3 in 82c8d31

	add_custom_target( build_hardfork_hpp
	COMMAND cat-parts "${CMAKE_CURRENT_SOURCE_DIR}/hardfork.d" "${CMAKE_CURRENT_BINARY_DIR}/include/graphene/chain/hardfork.hpp" )

If file A has a #define xxx as last line and file B has something meaningful as first line, after concatenated, the first line in B would become a part of the #define thus will break build.

[oxarbitrage]

A few more styles ideas obtained from https://github.com/bitshares/bitshares-core/pull/1419/files

• Avoid multiple empty new lines in code, in general 1 empty line is enough to separate blocks of code.
• Prefer FC_ASSERT instead of assert
• Remove commented code. Comments can be made when code seems not enough to explain an idea, that comments are genrally used in test cases and explain in plain English what is happening. Commented code should not be submitted, just delete it.
• In lambda, prefer capture variables explicitly if what you need is not too many.

[oxarbitrage]

From: #1413 (comment)

• In a huge refactoring please do it in small steps in individual commits that are easily verifiable. This makes the reviewer life a bit easier.

[oxarbitrage]

From #1449 (comment)

create/modify/delete of the database inside operation evaluator must be done in do_apply and never in do_evaluate

[pmconrad]

The last one is not really coding style but an architectural decision that is specific to our codebase.

[oxarbitrage]

Most of them are or should be specific to our codebase. Architectural ones i think should be included as well(maybe as a separate document or whatever).

It saves time to the programmer and reviewer if the coder know beforehand she should not be doing it that way.

[cogutvalera]

Most of them are or should be specific to our codebase. Architectural ones i think should be included as well(maybe as a separate document or whatever).

It saves time to the programmer and reviewer if the coder know beforehand she should not be doing it that way.

absolutely agree with you 👍

[oxarbitrage]

• Pull requests should add/modify/delete the minimum amount code possible to develop 1 and only 1 feature or concept. From add startup message to all plugins #1467 - Pull doing several stuff at once.

• Pull requests in bitshares can only be made against 2 branches: develop if there is a non consensus related change or hardfork if it is modifying consensus.

• From: add last_vote_time to account stats object #1449 (review) - When new fields are added or removed to objects; a bump of the database is needed.

[oxarbitrage]

From #1324 (comment):

• avoid global variables.

[ryanRfox]

@cedar-book are you willing to work with me on capturing this information into a style guide. Once a Core Team approved Style Guide exists (wiki?), then we can add some tools to clean up the existing code and keep it clean going forward. I know we will need some help from the Core Devs, but hope we can get a start on it for them.

[cedar-book]

Hi @ryanRfox, Yes, Of course. I would like to support!! Just let me know where to start.

[nathanielhourt]

Unpopular opinion: if we're going to formalize our style guidelines, we should fix the worst aspects of our style: migrate to 4 spaces of indentation, and do away with padding the insides of parentheses.

Personally, I'm a fan of

if (xyz) {

over

if (xyz)
{

as the latter eats vspace very quickly and gives no readability improvement in return, but I have no strong feelings on that one.

I also recommend not indenting namespaces, i.e. use

namespace abc {
bool foo() { return false; }
}

instead of

namespace abc {
    bool foo() { return false; }
}

and also not indenting access specifiers in classes, i.e. use

class foo {
public:
    foo() {}
};

instead of

class foo {
    public:
        foo() {}
};

These are major contributors to rightward drift, which does reduce code readability.

OK, let the holy whitespace wars commence! =)

[jmjatlanta]

I personally do not like rightward drift. So 3 spaces, do not save as tabs. Padding inside parenthesis I think adds to readability.

But I would much prefer to have standards than allow contributions that do not comply. So consider me a supporter of the standard, but complacent to what the standard is (within reason).

[pmconrad]

Maximum line length: 123(?) (that's what you can see on github without side-scrolling)

[abitmore]

Maximum line length: 123(?) (that's what you can see on github without side-scrolling)

Just checked, 118.

[nathanielhourt]

One of the major problems with padding the inside of parentheses is that it never gets applied consistently. Even in FC, it's not applied consistently, and there are plenty of places where that's not even my fault. It gets mishandled by any IDE that does automated code formatting, generation, or modification, and I doubt many dedicated auto-formatting tools support it.

Furthermore, I've never before seen it called for in any professional (C++) style guide. In 15 minutes of googling, I find exactly one style guide from the '90s that uses it, and I see that Herb Sutter uses it, and both of them use it inconsistently. Google does not do it. Microsoft does not do it. Apple does not do it. Facebook does not do it. Why are we still talking about this??

I also just so happen to think it's ugly as sin and makes our code look ridiculous, but even I can acknowledge that isn't a good argument against it. I would point out, nevertheless, that if we mandate the padding of the insides of parentheses, we will write countless hundreds of modification requests asking contributors to add it, whereas if we do not, we will probably never once have to ask a contributor to remove it. It's unfamiliar and clumsy to virtually all developers, and one need only read StackOverflow to know this is true.

OK, now I openly acknowledge that I have ranted about this to the point of absurdity, so I will STFU now and not address it again. Thank you for your patience.

[nathanielhourt]

And fwiw, my biggest problem with 3 space indentation is, again, NOBODY DOES IT. 4 spaces is virtually universal. Again, go read StackOverflow: 4 spaces is ubiquitous, sometimes you see 2. You never see 3. And it's a pain to reconfigure my editors to use 3 spaces when I work on BTS/FC vs. 4 on every other project ever (this is an exaggeration). Look at our 3rd party dependencies in FC's vendor subdir: 4 space indentation on all three.

Once again, I've said my piece. I promise to shut up now.

[oxarbitrage]

I agree with both from @nathanhourt last comments, specially the padding inside parenthesis, should be removed. I have not so many issues with the indentation, 4 seems to be more standard so i will agree with a change in that, i dont like 2 spaces.

If we all agree in this changes we need to decide on implementation of them. We have the option to make it by file, by folder or with a huge pull request against all the project. It will be a pain to review in case anything was done incorrectly by whatever.

It should be an automated process, any preference? Maybe just a bash script ?

Another thing that is related and will require modification on every project file is the license. It is recommend almost everywhere to use a central place and link there, similar to what eos is doing: https://github.com/EOSIO/eos/blob/master/libraries/chain/abi_serializer.cpp
IIRC we discussed this at some other ticket but maybe the right place to discuss it should be here.

[cedar-book]

@ryanRfox, I have collected several Coding Standard / Guide information. These are others but we can look at them as references. If we can find some good guide, we might be able to adjust and include the BitShares-Core coding style guide. (*I saved the below files in a temporary location.)
References: <1>, <2>, <3>
For BitShares Core: Draft
It's very draft. I added some categories to see how they look like in there. We can change and build up as we go.

[abitmore]

3-whitespace indentation is a rule in a company I've worked for some years before, so I would not say "nobody does it".. It benefits developers with a small screen (E.G. I had been coding on a 12-inch laptop for years), while not looking so crowded as 2 whitespaces.

As mentioned in #1629 (comment), whitespaces in the code are helpful when navigating in VIM with SHIFT + w key. For example, when writing code like

a_variable = a_function( a_parameter ).another_function( another_parameter );

we usually don't add a whitespace before (, thus a whitespace after ( is helpful and looks fine.

[pmconrad]

We should

• come to a consensus regarding formatting rules
• find a tool that can do it automatically
• make a big whitespace-only change

[cnjsstong]

I've done a bit research on the linter. Looks like cpplint is a widely used cross-platform linting tool (built in Python). The rules are configurable and the built-in rules are based on the Google C++ Style Guide. One disadvantage is that it does not have auto fix for simple rules (like that in ESLint).

There are other linting tools with better static analysis performance, but a lot of them are platform dependent.

IMHO the style guideline should more focus on readability and unambiguity for both human and compilers. So I would suggest to keep the parentheses even for single-statement if's, for example.

[ryanRfox]

Good discussion going on here. I'd like wrap this up in the next two weeks, if possible. To increase speed, I feel we should move the draft outline @cedar-book started in their repo to a Style Guide Wiki page (marked as "DRAFT")? That way any of us may edit the Wiki without needing to approve PRs. Will that work for the Team?

I agree with @pmconrad that we should perform these tasks:

• Consensus regarding formatting rules
• Identify a tool(s) that can do it automatically
• Perform whitespace-only change

I'm going to move that into the Description of this Issue and we can update it as we proceed.

[cedar-book]

@ryanRfox, I do not (cannot) edit a Wiki. - Thank you for moving!

[nathanielhourt]

Oh, one other suggestion: deprecate typedef in favor of using. This is recommended in the C++ Core Guidelines and is also, I think, far more readable.

[oxarbitrage]

#1765 (comment)

Using try-catch on the whole code block is not good practice.

[pmconrad]

Using try-catch on the whole code block is not good practice.

It depends on what you do in the catch block IMO.