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

Code cleanups #931

Merged
merged 2 commits into from
Aug 20, 2020
Merged

Code cleanups #931

merged 2 commits into from
Aug 20, 2020

Conversation

stebet
Copy link
Contributor

@stebet stebet commented Aug 20, 2020

Proposed Changes

Loads of code cleanups:

  • Explicit types instead of var where type is not apparent.
  • Removed private setters from properties where they were not used.
  • Simplified null-checks to x is null instead of x == null. Also prevents accidental bugs in case == operator would ever be overloaded.
  • Made private variables readonly where possible.
  • Simplified Properties and Methods to be expression-based where possible.
  • Made null-checks implit via property where it made sense to reduce code clutter.
  • Changed multiple ifs to switch where possible.

Types of Changes

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)

Checklist

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • All tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in related repositories

@stebet
Copy link
Contributor Author

stebet commented Aug 20, 2020

For an example of the most drastic changes and code reductions, see https://github.com/rabbitmq/rabbitmq-dotnet-client/pull/931/files#diff-0ea0e0e737f90115933e0196f709f5b8 for example.

@michaelklishin michaelklishin merged commit 54b3ad6 into rabbitmq:master Aug 20, 2020
@danielmarbach danielmarbach deleted the codeCleanups branch August 21, 2020 04:54
@@ -662,14 +440,12 @@ private void Init(IFrameHandler fh)
}
}


Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: extra line

lock (_eventLock)
{
_connectionShutdown -= value;
}
}
}


Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: extra line


public uint FrameMax { get; set; }

public TimeSpan Heartbeat
{
get { return _heartbeat; }
get => _heartbeat;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we even need the field here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe not. I haven't cleaned up fields and made auto properties where possible everywhere yet.

@@ -99,55 +99,62 @@ public static void DumpKeyValue(string key, object value, TextWriter writer, int
///<summary>Dump properties of objects to the supplied writer.</summary>
public static void DumpProperties(object value, TextWriter writer, int indent)
{
if (value == null)
switch (value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we lock the csharp version to not accidentally use something that would not be compatible?

Copy link
Contributor Author

@stebet stebet Aug 21, 2020

Choose a reason for hiding this comment

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

If you use the default csharp version it target the lowest common denominator based on the TargetFrameworks. Bit if needed we can lock it to 7.3 according to this: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/configure-language-version

@lukebakken lukebakken added this to the 7.0.0 milestone Aug 25, 2020
@michaelklishin
Copy link
Member

It would be very nice to have a version of this for 7.x: this makes the delta with master noticeably wider and complicates backporting of subsequent PRs.

@lukebakken lukebakken modified the milestones: 8.0.0, 7.0.0 Mar 8, 2022
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