Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Code style #858

Closed
wants to merge 6 commits into from
Closed

Fix: Code style #858

wants to merge 6 commits into from

Conversation

Jim8y
Copy link
Contributor

@Jim8y Jim8y commented Dec 13, 2023

Since discussed that the code style is a thing for the core, this pr fixs some of the code style issues that has being there for years.

Comment on lines +19 to +20
KNoCompression = 0x0,
KSnappyCompression = 0x1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove the K

@@ -56,16 +56,16 @@ protected override void OnSystemLoaded(NeoSystem system)
}

[RpcMethod]
public JToken GetApplicationLog(JArray _params)
public JToken GetApplicationLog(JArray @params)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parameters, value?

@@ -176,36 +176,36 @@ private void OnVerifyProof(UInt256 root_hash, string proof)
}

[RpcMethod]
public JToken GetStateRoot(JArray _params)
public JToken GetStateRoot(JArray @params)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here and in the rest of them, I don't like the @

Copy link
Contributor Author

@Jim8y Jim8y Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using @ for reserved words to use them as a variable is a C# standard. But I'll update it. And please, we are working on the core, not some personal project, please avoid using "like" or "not like", it sounds like you are using your personal feelings (though i know you are not) instead of professional judgements.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using @ for reserved words to use them as a variable is a C# standard. But I'll update it. And please, we are working on the core, not some personal project, please avoid using "like" or "not like", it sounds like you are using your personal feelings (though i know you are not) instead of professional judgements.

Could you show me where said that use a reserved word is a good practice?

Copy link
Contributor Author

@Jim8y Jim8y Dec 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not saying using reserved word is a good practice, cause if i were the one who wrote the code, i will not use params as the parameter. But it is params and is used as _params, which is definately not a good practice. You said we should have a good code style, yet you allow someone to use _params as the name of the parameter, this pr trys to fix the bad practice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Liaojinghui, you're trying to change some established code. _params was there since like forever, so for good or bad, but everyone just got used to it being this way. If all methods consistently use this name, no one can really object adding another one using the same style, that's just consistency. Is it a bad name? Sure it is. But just making it @params is not an improvement to me, at least not worth the trouble.

parameters is a different story, nice to have it instead and have it used consistently for every method as well. 👍

Copy link
Member

@cschuchardt88 cschuchardt88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know you can use the analyzers in Visual Studio to do this all for you automatically. Also we should be using the .editorconfig so this doesn't happen again. See #826

@@ -34,7 +34,7 @@ public DBFTPlugin()

public DBFTPlugin(Settings settings) : this()
{
this.settings = settings;
this._settings = settings;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should remove this. Dotnet coding standards, 99% of professional c# developers dont use that method anymore.

@@ -44,55 +44,55 @@ public override void Dispose()

protected override void Configure()
{
settings ??= new Settings(GetConfiguration());
_settings ??= new Settings(GetConfiguration());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should remove the class. so it would be

_settings ??= new(GetConfiguration());

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not matter. Many places keep it, many remove it.

@Jim8y
Copy link
Contributor Author

Jim8y commented Dec 19, 2023

You know you can use the analyzers in Visual Studio to do this all for you automatically. Also we should be using the .editorconfig so this doesn't happen again. See #826

@cschuchardt88 Only if someone does it. Anything in neo core can take years.

@Jim8y
Copy link
Contributor Author

Jim8y commented Feb 5, 2024

close as not planed, may work with editorconfig

@Jim8y Jim8y closed this Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants