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

Add ToString methods for better enum string conversion #3458

Merged
merged 2 commits into from
Sep 17, 2024

Conversation

paulomorgado
Copy link

Description

  • Added private ToString methods for various comparison enums in ConditionFactory.cs to improve readability and maintainability.
  • Updated NewCondition methods to use these new ToString methods for consistent string representation.
  • Optimized boolean conversion to string using a ternary operator.

Motivation and Context

The result of invoking ToString on an enum is cached in a limited space and there's no guarantee that the value will be cached for the same value in consecutive invocations.

Having code that returns compile-time generated static strings will guarantee that that string is always available and easy to access.

Invoking ToString in bool will return the stringliterals "True" or "False", depending on its value. However, invoking ToLowerInvariant() on those Strings will incur in the generation/allocation of a new string every time.

Testing

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed

License

  • I confirm that this pull request can be released under the Apache 2 license

@paulomorgado
Copy link
Author

Tests invoking AssertExtensions.AssertEnumUnchanged should be added to validate changes to these enums, but I haven't figured out what the best place for it would be.

@dscpinheiro dscpinheiro changed the base branch from main to v4-development August 27, 2024 19:09
@danielmarbach
Copy link
Contributor

Nice change @paulomorgado

@paulomorgado
Copy link
Author

Nice change @paulomorgado

Thanks, @danielmarbach.

Maybe introducing NetEscapades.EnumGenerators: a source generator for enum performance would be a good option to avoid hand-written code.

Copy link
Contributor

@peterrsongg peterrsongg left a comment

Choose a reason for hiding this comment

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

The change itself looks good, but these need unit tests
I think this is a good place to put the tests.

I think just testing a few is sufficient, probably no reason to test all of them but I would feel more comfortable knowing we had unit tests.

I wrote a method to get the expected hash of the original type

    public static class HashingUtils
    {
        private static SHA256 SHA256 = SHA256.Create();
        public static string GetHash(Type type)
        {
            return BitConverter.ToString(SHA256.ComputeHash(Encoding.Default.GetBytes(type.ToString()))).Replace("-","");
        }
    }

which you could use to get the expected hash and then pass that in to AssertEnumUnchanged. You may have to tweak it a little

@paulomorgado
Copy link
Author

paulomorgado commented Sep 14, 2024

The change itself looks good, but these need unit tests I think this is a good place to put the tests.

I think just testing a few is sufficient, probably no reason to test all of them but I would feel more comfortable knowing we had unit tests.

I wrote a method to get the expected hash of the original type

    public static class HashingUtils
    {
        private static SHA256 SHA256 = SHA256.Create();
        public static string GetHash(Type type)
        {
            return BitConverter.ToString(SHA256.ComputeHash(Encoding.Default.GetBytes(type.ToString()))).Replace("-","");
        }
    }

which you could use to get the expected hash and then pass that in to AssertEnumUnchanged. You may have to tweak it a little

Done, @peterrsongg!

Added private ToString methods for various comparison enums in ConditionFactory.cs to improve readability and maintainability. Updated NewCondition methods to use these new ToString methods for consistent string representation. Also, optimized boolean conversion to string using a ternary operator.
Updated PolicyTests.cs to include additional namespaces and
added new test methods to verify the integrity of various enums
by comparing their hashes to expected values. Introduced a new
configuration file, AWSSDK.NetFramework.lutconfig, to specify
build and test settings, including enabling parallel builds and
setting a test case timeout.
@96malhar 96malhar merged commit e7ff3a9 into aws:v4-development Sep 17, 2024
@paulomorgado paulomorgado deleted the performance/enum-tostring branch September 17, 2024 17:28
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