Skip to content

Conversation

@pvdb
Copy link
Contributor

@pvdb pvdb commented Aug 25, 2024

Very much in the same vein as the refactor in #29 this small PR makes the two "converter" lookups (for factor and offset respectively) slightly more idiomatic and - arguably - a tad simpler: instead of using Hash#[] it now uses Hash#fetch with - where appropriate - meaningful defaults.

To verify that this refactor is functionally equivalent to the original implementation, I have also included some specs for the Converters module, which all run "green" on both the master branch, as well as on the feature branch for this pull request.

The new implementation also avoids superfluous hash lookups: first to test the error conditions, and then exactly the same lookups again to get the actual conversion factor or conversion offset ... this now happens in one go.

The only (minor?) difference introduced by this PR is the wording of the two error messages that are logged when a lookup error occurs, which can be illustrated with:

git rev-diff converters_refactor^^ converters_refactor rspec -I lib -r fit4ruby -fd spec/Converters_spec.rb

The first lookup error message changes as follows:

-No conversion factors defined for unit 'F' to 'C'
+No conversion factor defined from unit 'F' to unit 'C'

... and the second one as follows:

-No conversion factor from 'C' to 'K' defined.
+No conversion factor defined from unit 'C' to unit 'K'

... but as far as I can tell this is pretty much inconsequential 🤞

Thanks, @scrapper! 🙏

@pvdb pvdb changed the title Converters refactor refactor the Converters module Aug 26, 2024
@pvdb pvdb force-pushed the converters_refactor branch from c98d56c to b531c57 Compare August 26, 2024 06:47
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.

1 participant