Skip to content

Conversation

@nwithan8
Copy link
Contributor

Description

  • New SmartrateAccuracy enum (similar to C# library)
  • New functions to use SmartrateAccuracy when dealing with smartrates
  • Deprecate old smartrate functions using strings

Testing

  • Unit tests updated to use new functions
  • No new unit tests
  • No new cassettes needed
  • No new recordings needed

Pull Request Type

Please select the option(s) that are relevant to this PR.

  • 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 not work as expected)
  • Improvement (fixing a typo, updating readme, renaming a variable name, etc)

- New functions to use SmartrateAccuracy when dealing with smartrates
- Deprecate old smartrate functions using strings
@nwithan8 nwithan8 requested review from Justintime50 and jchen293 and removed request for jchen293 June 29, 2022 00:24
@nwithan8 nwithan8 marked this pull request as ready for review June 29, 2022 00:24
Copy link
Member

@Justintime50 Justintime50 left a comment

Choose a reason for hiding this comment

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

Let's ask a genuine question here: "what does this refactor accomplish that the original did not"? If we are refactoring for the sake of refactoring, we shouldn't do it. I'd like to point out that this takes ~75 extra lines of code to ultimately do the same thing as the current implementation. Enums are good but at what cost if (at least this case) they introduce so much extra code that does the same thing while deprecating a function for the sake of having the ability to use an enum.

Thoughts?

@nwithan8
Copy link
Contributor Author

nwithan8 commented Jun 29, 2022

Let's ask a genuine question here: "what does this refactor accomplish that the original did not"? If we are refactoring for the sake of refactoring, we shouldn't do it. I'd like to point out that this takes ~75 extra lines of code to ultimately do the same thing as the current implementation. Enums are good but at what cost if (at least this case) they introduce so much extra code that does the same thing while deprecating a function for the sake of having the ability to use an enum.

Thoughts?

The main purpose of enums is for when there are a limited number of options available for the end user to use, such as here where there are only a handful of valid smartrate accuracies to choose from.

Inherently, using enums prevents errors.

Compared to hardcoded strings, they prevent errors due to accidental misspellings, inconsistencies in casing, etc. Functions that accept a enum inherently require the user to pass in a valid value, making it virtually impossible for a user to pass an invalid value as a parameter to a function. Plus, those using code completion can easily pull up the list of available enums to them, rather than having to refer to our documentation to find the exact string to use in a specific function. In that way, they act almost as constants.

There's also a variety of underlying benefits to using enums rather than int-to-int or string-to-string comparisons in terms of compilation, functionality and safety.

Particularly in Java, enums can also hold internal values and data (here, the actual word we need to use is stored); the benefit being that the end-user never needs to be aware of this data. Unfortunately, this isn't native in C# or some other languages (I know Python doesn't do it either), which led me to having to roll my own Enumeration class in C#.

In terms of the extra code, to be fair, I believe some of these methods that are marked as deprecated here haven't been officially released yet, so we can trim some of them down without introducing any breaking changes. The SmartrateAccuracy enum class itself is only 47 lines. And functions like "match by value" are handled internally by the enum, rather than having to rewrite that logic in each function accepting a smartrate accuracy string.

Justintime50
Justintime50 previously approved these changes Jun 29, 2022
Copy link
Member

@Justintime50 Justintime50 left a comment

Choose a reason for hiding this comment

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

A well defended stance on enums 🙂 I can accept that.

@nwithan8 nwithan8 requested a review from jchen293 June 29, 2022 21:43
@nwithan8 nwithan8 merged commit 9c0db66 into master Jun 30, 2022
@nwithan8 nwithan8 deleted the percentile_enums branch June 30, 2022 22:53
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