Code style policy changes

[Aprogiena]

The purpose if this issue is to open discussion about changing coding style - https://github.com/stratisproject/StratisBitcoinFullNode/blob/master/Documentation/coding-style.md

Currently we only ask contributors to follow the coding style, yet we do not have any rules on whether or not a code can be accepted if it violates the coding style.

The current code base uses different code styles and a lot of code violates many rules of the current style.

I think we should define both - the rules we want (as the current rule set is not sufficient) as well as the policy about how this is or is not enforced - i.e. which rules are required and which are only recommended.

This issue is for StratisBitcoinFullNode project only at this moment, and once we converge to something here, other projects are welcome to follow the outcome as well.

Besides the rules and the policies on their enforcement, we should also define the decision process of which rules will be applied should contributors disagree on some (i.e. who is the boss if any, are there any vetos, or is this just majority voting?).

PROPOSING RULE (NEW OR CHANGE): If you want to propose a new rule, or change the text of the current rule, please start your comment starting with RULE PROPOSAL and use three sections: DEFINITION - in this first part just write the rule as you would like to see it; RATIONALE - try to explain why you think this is a good idea to have; POLICY - explain what kind of policy you would like to have on this rule (i.e. would you like it to have only as a general guideline/recommendation, or strictly enforced - i.e. not accepting new code violating it?).

[dangershony]

Indeed the policy is if you are working on some part of the code that they styling is different then bring them in line.

Code should not be accepted if it violates the code-style but it wasn't an iron rule (for various reasons).

My take on adding optional and strict on some of the rules seem fine, some contributors are used to other styling from other projects and we can make it easy to contribute by not staying too strict

[Aprogiena]

Do you think someone would actually have a problem if in the PR review the reviewer would say "here and here, please fix the style according to code style"? I've seen you already do that in the review process, and I think it is the right think to do - you have a code style for a reason. For example, I have 2 space indentation by default in MSVS, but I changed that to 4 to comply with the project code style. I can't imagine it would be acceptable if I started submitting code with 2 spaces. It would just create messy code.

[Aprogiena]

RULE PROPOSAL Properties with trivial or automatic getters and setters should be written on a single line . Trivial means return x; or x = value;. Initialization of the properties must be done in constructors.

Examples: public int X { get; set; } public int X { get; private set; } public BlockPuller Puller { get { return this.puller; } } public BlockPuller Puller => this.puller;

Forbidden: public int MaxItems { get; set; } = 100000;

RATIONALE Class (auto)properties are currently written in all different kinds of forms and it looks very inconsistent in the code. We do have probably all possible variants in the code from the above mentioned one liners to

        public bool Whitelisted
        {
            get; set;
        }

or

        public bool Whitelisted
        {
            get;
            internal set;
        }

or

        public bool Whitelisted
        {
            get
            {
                 return this.whitelisted;
            }
        }

POLICY Strict / Hard

[bokobza]

from our coding style guide:

12. We use PascalCasing to name all our constant local variables and fields.

[Aprogiena]

tx, i missed that, i deleted that proposal

[dangershony]

Single line getters and setters is fine with me I will add it to the styling guideline and also add the example (we might need to update the current example so I can just take the BlockPuller)

One more thing I would suggest that default setting should always go in a constructor, it is very annoying when debugging some code with setters directly on properties.

So to avoid this: public int MaxItems { get; set; } = 100000;

Instead do

public int MaxItems { get; set; }
public constructor()
{
     this.MaxItems = 100
}

[dangershony]

I also think this is actually ok

        public bool Whitelisted
        {
            get
            {
                 return this.whitelisted;
            }
        }

[Aprogiena]

The point of this issue is to avoid changing the code style guideline every so often. Rather to give it some time here to come with new rules and then make the change at once.

As for

 public int MaxItems { get; set; } = 100000;

I'm OK to forbid that. But as @bokobza gave a thumbs up on the original proposal that allows this, let him have a say before I edit the proposal.

As for

        public bool Whitelisted
        {
            get
            {
                 return this.whitelisted;
            }
        }

this seems longwinded and actually the proposed rule forbids it in favor of these:

public BlockPuller Puller { get { return this.puller; } }
public BlockPuller Puller => this.puller;

[bokobza]

yeah I don't like public int MaxItems { get; set; } = 100000; either. When you debug, unless all the properties are at the top (where they should be really), your debugger jumps up and down. I no like.

This one is my favorite: public BlockPuller Puller { get { return this.puller; } } I'm not used to this yet: public BlockPuller Puller => this.puller; But I'm fine with either way.

[Aprogiena]

Cheers, proposal upgraded to forbid that initialization.

[Aprogiena]

RULE PROPOSAL Parentheses in expressions are mandatory except for simple terms, such as x, x.field, or x.Method() etc., where they are forbidden. Multiple parentheses around a single expression are also forbidden.

Examples: if ((x.Value > 5) && y && z.Eval() && ((a + b) > c)) bool q = (x.Value > 5) && y && z.Eval() && ((a + b) > c); bool q = z.Eval(); return x == 1;

Forbidden: if (x.Value > 5 && y) if ((x.Value > 5) && (y)) if ((x.Value > 5) && (z.Eval())) if (((x.Value > 5))) if (y && a + b > c) if (y && (a + b > c)) if (y && (a + b) > c) bool q = (x.Value > 5); bool q = (y); bool q = (z.Eval()); return (x == 1); return (x) == 1; return x == (1);

RATIONALE Use of parentheses that are not required by the language are valuable because they capture intent of the developer. Otherwise it is hard to distinguish whether the developer is just ignorant of the operator priority or if the code works as intended. In some (rare) cases of refactoring, not using all parentheses could change the code behavior silently.

POLICY Strict / Hard

[bokobza]

Agree on this.

[bokobza]

RULE PROPOSAL Interfaces must start with an "I".

RATIONAL It's the common practice and this way the code is more homogeneous.

POLICY HARD

[Aprogiena]

@bokobza: I'd probably add to that rule that we have "-Async" suffix for async methods and "-Locked" suffix for methods that require the caller to hold one or more locks. For async the rationale is the same. For locks it is great prevention of bugs.

[bokobza]

agree with these. Also "[typename]Extensions" for extensions methods. What about convention for abstract classes? I vote for not using "-Base" but rather having a name that describe the object properly, like Vehicle or Shape.

[Aprogiena]

Yes for Extensions.

I actually use -Base, I don't see any cons with it. Maybe we are not that much different here. You probably want Triangle: Shape, where I would very much agree with Shape, except it would be ShapeBase or ShapesBase

[dangershony]

RULE PROPOSAL

Make test method names more readable

RATIONAL

Currently methods names are not so readable. I propose to change any name to have an '_' between words This is less readable

        [Fact]
        public void WalletCanReceiveAndSendCorrectly()
        {

This is more readable

        [Fact]
        public void Wallet_Can_Receive_And_Send_Correctly()
        {

Additionally we may want to start adding the 'Given – When – Then' syntax to each method. Example:

 [Fact]
        public void Given_a_valid_wallet_When_a_trannsaction_is_created_Then_it_can_send_and_Receive()
        {

POLICY

HARD

@Neurosploit thoughts?

[Aprogiena]

@dangershony In testing guidelines, we currently have this:

The test method name follows the following structure: {MethodName}{GivenContext}{ExpectedOutcome}. Example for a method called Query: QueryWithoutInitializedRepositoryThrowsException.

I think many tests are not complying with this. I tried to comply with that creating this test method

AssignBlocksToPeersWithNodesWithDifferentChainsCorrectlyDistributesDownloadTasks

and I don't think it is more readable if you add after every word there. However, I think the best format here would be `{MethodName}{GivenContext}_{ExpectedOutcome}`, which would make this method

AssignBlocksToPeers_WithNodesWithDifferentChains_CorrectlyDistributesDownloadTasks

which is probably best. I think it is better than AssignBlocksToPeersWithNodesWithDifferentChainsCorrectlyDistributesDownloadTasks as it ads separations between the logical parts of the name and it is better than Assign_Blocks_To_Peers_With_Nodes_With_Different_Chains_Correctly_Distributes_Download_Tasks exactly for the very same reason - the separation here has not separated the logical parts.

So I suggest that existing guideline to be changed to {MethodName}_{GivenContext}_{ExpectedOutcome} instead of what you suggested.

[dangershony]

Testing some readable scenarios: Option 1:

Assign_Blocks_To_Peers_With_Nodes_With_Different_Chains_Correctly_Distributes_Download_Tasks

Option 2:

AssignBlocksToPeers_WithNodesWithDifferentChains_CorrectlyDistributesDownloadTasks

Option 3:

Assign_Blocks_To_PeersWith_Nodes_With_Different_ChainsCorrectly_Distributes_Download_Tasks

Option 4:

Givenwe_assign_blocks_to_peersWhennodes_are_in_different_chainsThen__correctly_distribute_download_tasks

Option 5:

Given_AssignBlocksToPeers_When_NodesWithDifferentChains_Then_CorrectlyDistributesDownloadTasks

The most important thing is that the test method is instantly encapsulates its purpose and is very easy t o read (considering we will have a lot of tests)

[bokobza]

My favorites are 2 or 5.

[Neurosploit]

I agree with option 2 or 5. Preferring option 2.

[Aprogiena]

2 for me as well, second place also 5.

[dangershony]

My opinion here is that option 4 is most readable:

Givenwe_assign_blocks_to_peersWhennodes_are_in_different_chainsThen__correctly_distribute_download_tasks

I think tests methods should not be neat but very easy to read and quickly convey the story of the test.

If a test has more context option 2 becomes also hard to read:

AssignInvalidBlocksToSlowDownloadingPeers_WithNodesWithDifferentChains_CorrectlyDistributesDownloadTasks

Given__we_assign_invalid_blocks_to_slow_downloading_peersWhenthe_nodes_have_different_chainsThencorrectly_distributes_the_download_tasks

[Given]xxx[When]xxx[Then] adheres to BDD style of testing, I am going to discuss with the product managers if they want it we go with 5 else, I see a preference with option 2.

[Aprogiena]

I see [Given]xxx[When]xxx[Then] as problematic because if I open Test Explorer in MSVS, I see all tests and if I am looking for a specific test there (which is quite often), having the method name being the first thing there is the key. If the first thing there is "Given__" followed by unpredictable sentence (looking for a test I don't really know what should follow by Given) I won't be able to find it.

So if you want to go with 4 or 5, then please consider 6 first:

AssignInvalidBlocks_Givenwe_assign_blocks_to_peersWhennodes_are_in_different_chainsThen__correctly_distribute_download_tasks

i.e. {MethodName}_[Given]xxx[When]xxx[Then]xxx

[dangershony]

Interesting, putting the [key] first (method name being the key). I like that very much actually, you get a proper separation by key in the explorer and still keep the test story.

I will try this with the FundTrasnaction tests I am currently writing and we can see if it feels right.

[Aprogiena]

Yes, but in my opinion tests should be commented just as any other code (see that AssignBlocksToPeersWithNodesWithDifferentChainsCorrectlyDistributesDownloadTasks example). I don't think that trying to put all the description into the test name is useful (if comments are there).

So I'd rather see less descriptive test method names and comments for tests describing the scenarios. The current test code base we have is hugely lacking comments of the tests.

[Aprogiena]

It might even make sense to have module first {Module/Component Name}{MethodName}... for even better navigation in the test explorer.

[Aprogiena]

Thinking of this, many people here liked option 2:

AssignBlocksToPeers_WithNodesWithDifferentChains_CorrectlyDistributesDownloadTasks

but that also suffers from not having Module/Component name in the first place, so in fact in test explorer, you are still kind of lost, unless you remember which module implements which methods

So let's say option 7

PullerDownloadAssignments_AssignBlocksToPeers_WithNodesWithDifferentChains_CorrectlyDistributesDownloadTasks

[bokobza]

The names of these methods are getting out of hand in my opinion. How about adding categories/module to the tests as traits? We can already filter by namespace, class, traits and even search by name from within the test explorer, I don't think we need other visual clues.

[Aprogiena]

OK, that's true, possibly this long list of all tests is maybe not that important

[bokobza]

Come on I didn't say the list is not important, I said we can make it as navigable without having to resort to longer names.

[Aprogiena]

No I mean I agreed with you and possibly changed my mind about the naming - I was previously not using the search, now trying it seems good enough to find the tests you are looking for, so that seems like satisfying my primary requirements.

[bokobza]

Cool then. Sorry mate, it's hard to see if someone is speaking frankly or being sarcastic online 😸

[dangershony]

Normally we don't put descriptions on tests, so that's why putting description on the method name is more important.

Apparently there are different ways and many discussions https://stackoverflow.com/questions/155436/unit-test-naming-best-practices We need to decide on one way.

People might first complain that the name of the test method appears way too long for them, but in the end it s more about readability. Longer test names do not imply longer or less optimized code, and even if they did, we are not looking to optimize test code performance. If anything, test code should always be optimized for readability.

I am not fixed on using Given_When_Then

Example:

[Aprogiena]

@bokobza no worries :) but yes, no sarcasm was intended

@dangershony and I think that is a mistake "not to put descriptions on tests", the very same arguments for commenting your code applies here - describe what you intend to do, so that someone can check that you implemented it right

[dangershony]

hi @detroitpro would you like to make a recommendation?

[mikedennis]

These look good to me. One suggestion if you don't want to have too many rules is to have a blanket statement something along the lines of "Unless explicitly mentioned in these rules please use the .NET core library style." That way you don't have to have a rule for every little thing.

BTW I'm not sure if you guys have seen this but here is some of MS rules https://github.com/aspnet/Home/wiki/Engineering-guidelines#coding-guidelines

[Aprogiena]

Those ASP.NET MS rules are terrible for me and I think so they are for @dangershony because they very much contradict to what this project had before I even joined :-)

So I'm OK to have the ruleset based on external template, but certainly not that one :-)

[dangershony]

Yeah I am very much against many of the asp.net rules.

But I might adopt their test naming policy (or lack of policy)

Unit test method names must be descriptive about what is being tested, under what conditions, and what the expectations are. Pascal casing and underscores can be used to improve readability. The following test names are correct: PublicApiArgumentsShouldHaveNotNullAnnotation Public_api_arguments_should_have_not_null_annotation

https://github.com/aspnet/Home/wiki/Engineering-guidelines#unit-test-method-naming perhaps the that's a good approach we dont restrict test method name and allow anything that makes sense as long as the test method name is descriptive and readable.

Also rule 6 of the guidelines seems a nice one to adopt

Use any language features available to you (expression-bodied members, throw expressions, tuples, etc.) as long as they make for readable, manageable code. This is pretty bad: public (int, string) GetData(string filter) => (Data.Status, Data.GetWithFilter(filter ?? throw new ArgumentNullException(nameof(filter))));

https://github.com/aspnet/Home/wiki/Engineering-guidelines#coding-style-guidelines--general

[Aprogiena]

On the rule 6 you've mentioned, I'd be much happier if the logic was inverse there:

Don't use language features available to you (...) unless they make for readable, manageable code.

[bokobza]

Happy with not having strict rules on the tests methods names, as long as it's descriptive enough as in the rule above.