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

Standardize and Optimize Code with dotnet format #3754

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

TiKevin83
Copy link
Contributor

@TiKevin83 TiKevin83 commented Aug 20, 2023

Follow up on #3681

Proposes merging the corrected output of dotnet format style -v detailed --severity info, optimizing and/or simplifying code per dotnet standards and removing unused code.

Implements or standardizes the following:

  • destructuring assignment including in foreach initialization and in combination with tuples for concurrent assignment operations, removing the need for temp variables
  • shorthand new() that doesn't repeat the variable type
  • new switch syntax
  • new using syntax
  • new local function syntax
  • |=, &=, >>=, <<=, ++, --, +=, -=, %=, ??= operator usage
  • consistent readonly usage
  • removes unused/commented out code
  • lots of simplified boolean logic
  • use of the is and is not keywords in favor of == and != where relevant
  • array access from end with [^]
  • array range operator with [..] (also simplifies many substring uses)
  • optional chaining with ?.

potential outlying changes:
There's a change to RCheevos.cs that looks possibly intended to be the existing way
Some removals of unused code in the Amstrad CPC, Commodore 64, PC Engine, and Sinclair Spectrum CPU cores may not make sense (it looks like a bunch of cpu registers end up unused)

Check if completed:

Copy link
Member

@YoshiRulz YoshiRulz left a comment

Choose a reason for hiding this comment

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

This would be a good idea to enable—after the codebase has been given a manual once-over. This way is like seeing a dead canary and deciding to pump clean air into the birdcage.

@TiKevin83
Copy link
Contributor Author

This would be a good idea to enable—after the codebase has been given a manual once-over. This way is like seeing a dead canary and deciding to pump clean air into the birdcage.

I don't think the order is that crucial. I did look briefly before and found I can definitely improve the codebase with a manual review in ways that the auto checks can't find with things like LINQ expressions, so I could work on that first though if preferred. This PR also doesn't strictly enforce following dotnet format as a pre-commit or CI check which could be desired if the manual combing is also in place.

@tommai78101
Copy link
Contributor

Would it be helpful to add some info about the formatting style to the CONTRIBUTING file, and also the suggested / recommended way of checking for the formatting style based on the outputs of the dotnet style? https://github.com/TASEmulators/BizHawk/blob/master/contributing.md

@TiKevin83
Copy link
Contributor Author

Would it be helpful to add some info about the formatting style to the CONTRIBUTING file, and also the suggested / recommended way of checking for the formatting style based on the outputs of the dotnet style? https://github.com/TASEmulators/BizHawk/blob/master/contributing.md

Definitely would be helpful to add that to Contributing however "format" is a bit misleading here, this PR is less intended to change formatting standards like indent style and more making use of new language features where the format command happens to know how to implement them automatically.

@tommai78101
Copy link
Contributor

tommai78101 commented Aug 22, 2023

I see. I look forward to seeing the new info added to CONTRIBUTING.

@@ -52,7 +52,7 @@ public virtual bool StartsFromSavestate
public bool StartsFromSaveRam
{
// ReSharper disable SimplifyConditionalTernaryExpression
get => Header.TryGetValue(HeaderKeys.StartsFromSaveram, out var s) ? bool.Parse(s) : false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should be reverted, as even though it's technically a "simplification" it makes the code less readable. The resharper disable is already evident of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree on this one, usually something is less readable/over complicated if you're returning true in the true condition of a ternary or likewise false in the false condition, but if someone is already intentionally ignoring the optimization then sure we can revert it

@TiKevin83 TiKevin83 force-pushed the more-dotnet-format branch 2 times, most recently from bf09f1e to 46e988b Compare August 30, 2023 23:52
@TiKevin83
Copy link
Contributor Author

Rebased on Morilli's PR and reran against the analyzer setting changes. I think something might be wrong with the IDE0007/IDE0008 var vs explicit settings still, will play with that as I have time.

@Morilli
Copy link
Collaborator

Morilli commented Aug 31, 2023

I think the main issue here is that a lot of the inspections at suggestion level only really work as just that: A suggestion.

Blanket-applying those everywhere unconditionally may not be desired in all cases.

@TiKevin83
Copy link
Contributor Author

for sure I'll toy with it more, I'll probably rerun it at warning level and retune individual rule warning levels if I see that missing more than I think people would agree on

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.

5 participants