search

bitshares / bitshares-core

BitShares Blockchain node and command-line wallet
https://bitshares.github.io/
Other
1.17k stars 647 forks source link

Define coding style guidelines #1318

Open pmconrad opened 6 years ago

pmconrad commented 6 years ago

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

Impacts

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

CORE TEAM TASK LIST

cogutvalera commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

Some basics rules:

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago 
* newline at the end of each file.

Pure curiosity... Why?

abitmore commented 6 years ago

Pure curiosity... Why?

Probably due to build scripts like this used in the project: https://github.com/bitshares/bitshares-core/blob/82c8d3139e2df5d94f8a4802b8742d9f0f7045c8/libraries/chain/CMakeLists.txt#L2-L3

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 commented 5 years ago

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

oxarbitrage commented 5 years ago

From: https://github.com/bitshares/bitshares-core/pull/1413#issuecomment-437932230

oxarbitrage commented 5 years ago

From https://github.com/bitshares/bitshares-core/pull/1449#discussion_r236381016

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

pmconrad commented 5 years ago

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

oxarbitrage commented 5 years ago

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 commented 5 years ago

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 :+1:

oxarbitrage commented 5 years ago
oxarbitrage commented 5 years ago

From https://github.com/bitshares/bitshares-core/pull/1324#issuecomment-439715251:

ryanRfox commented 5 years ago

@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 commented 5 years ago

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

nathanielhourt commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

abitmore commented 5 years ago

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

Just checked, 118.

nathanielhourt commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 https://github.com/bitshares/bitshares-core/pull/1629#discussion_r265062960, 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 commented 5 years ago

We should

cnjsstong commented 5 years ago

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 commented 5 years ago

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:

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

cedar-book commented 5 years ago

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

nathanielhourt commented 5 years ago

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 commented 5 years ago

https://github.com/bitshares/bitshares-core/pull/1765#discussion_r285395073

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

pmconrad commented 5 years ago

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

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