Skip to content

feat: add temperature unit conversions #5315

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

Merged

Conversation

vil02
Copy link
Member

@vil02 vil02 commented Aug 10, 2024

This PR adds a framework to work with unit conversions. I did not want to make the things too complicated so I only handle the case, when relation between units is linear.

Please review this very carefully, because probably many people would like to add more types of unit conversions in the future. Please pay special attention to:

  • UnitConversionsTest - I did not want to make it too complicated. It is not perfect, because adding a new quantity (like mass, length, time etc.) requiters adding 3 methods. But I think that it is still reasonable,
  • UnitConversions - my idea is to have static members like TEMPERATURE, LENGTH, MASS, TIME etc. there. Feels a bit weird to me, but nothing else comes to my mind,
  • UnitsConverter.computeAllConversions - probably it not the most efficient way, but there are not that many units in general. I guess it can be optimized in the future without changing the API,
  • is it fine to represent units as Strings?

Please let's do not rush here.

This PR is partially inspired by #5048.

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized it.
  • All filenames are in PascalCase.
  • All functions and variable names follow Java naming conventions.
  • All new algorithms have a URL in their comments that points to Wikipedia or other similar explanations.
  • All new code is formatted with clang-format -i --style=file path/to/your/file.java

@vil02 vil02 requested a review from siriak August 10, 2024 12:57
@vil02 vil02 force-pushed the add_temperature_converter branch from b2845f9 to c297ed8 Compare August 10, 2024 13:00
@codecov-commenter
Copy link

codecov-commenter commented Aug 10, 2024

Codecov Report

Attention: Patch coverage is 98.24561% with 1 line in your changes missing coverage. Please review.

Project coverage is 45.65%. Comparing base (5149051) to head (97e90a7).

Files Patch % Lines
...com/thealgorithms/conversions/AffineConverter.java 88.88% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #5315      +/-   ##
============================================
+ Coverage     45.44%   45.65%   +0.20%     
- Complexity     2817     2840      +23     
============================================
  Files           522      525       +3     
  Lines         15368    15425      +57     
  Branches       2915     2921       +6     
============================================
+ Hits           6984     7042      +58     
+ Misses         8090     8089       -1     
  Partials        294      294              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vil02 vil02 marked this pull request as ready for review August 10, 2024 13:05
@vil02 vil02 force-pushed the add_temperature_converter branch from c297ed8 to 5d61e90 Compare August 10, 2024 17:18
Copy link
Member

@BamaCharanChhandogi BamaCharanChhandogi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@BamaCharanChhandogi BamaCharanChhandogi enabled auto-merge (squash) August 21, 2024 11:08
@BamaCharanChhandogi BamaCharanChhandogi merged commit 07dbc51 into TheAlgorithms:master Aug 22, 2024
4 checks passed
@vil02 vil02 deleted the add_temperature_converter branch September 30, 2024 15:51
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