Skip to content

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
}
}


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

}
}


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 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.

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
Contributor

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