Closed EEM86 closed 1 year ago
Merging #107 into develop will decrease coverage by
5.26%
. The diff coverage is59.82%
.
@@ Coverage Diff @@
## develop #107 +/- ##
=============================================
- Coverage 86.01% 80.75% -5.27%
- Complexity 229 244 +15
=============================================
Files 35 39 +4
Lines 522 613 +91
Branches 32 44 +12
=============================================
+ Hits 449 495 +46
- Misses 51 93 +42
- Partials 22 25 +3
Impacted Files | Coverage Δ | Complexity Δ | |
---|---|---|---|
...g/sporttooolbox/converters/KilometerConverter.java | 38.09% <38.09%> (ø) |
4.00 <4.00> (?) |
|
...zwerg/sporttooolbox/converters/MilesConverter.java | 38.09% <38.09%> (ø) |
4.00 <4.00> (?) |
|
...rg/sporttooolbox/converters/NauticalConverter.java | 38.09% <38.09%> (ø) |
4.00 <4.00> (?) |
|
...egelzwerg/sporttooolbox/iunits/distance/Miles.java | 84.61% <75.00%> (-3.39%) |
11.00 <4.00> (ø) |
|
...zwerg/sporttooolbox/iunits/distance/Kilometer.java | 85.71% <80.00%> (-3.18%) |
12.00 <4.00> (ø) |
|
...ttooolbox/converters/DistanceConverterService.java | 86.36% <86.36%> (ø) |
3.00 <3.00> (?) |
|
...lzwerg/sporttooolbox/iunits/distance/Nautical.java | 88.88% <93.75%> (-1.74%) |
13.00 <9.00> (ø) |
|
...va/segelzwerg/sporttooolbox/iunits/speed/Knot.java | 93.33% <100.00%> (ø) |
11.00 <1.00> (ø) |
|
...lzwerg/sporttooolbox/iunits/speed/MilePerHour.java | 93.33% <100.00%> (ø) |
11.00 <1.00> (ø) |
|
... and 3 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 184435b...4df35ad. Read the comment docs.
Here are my suggestions to get the code more uncoupled.
After added converters and converter service new possible distance objects could use with ease convert methods and no need to create another method in Interface and in its implementations. Also, I believe in a sense of single responsibility convert methods should be managed with convert service.
I know why getters of Distance instances were private, but I suggest making them public to get the possibility for retrieving them from other objects.
I changed float types of variables to double as I believe it correlates with best practices due to float restrictions.
Still not sure that calculates are correct. Please, check different values in tests and create a task if you agree with me.
Will be waiting for your opinion on the mentioned suggestions.
Here are my suggestions to get the code more uncoupled.
- After added converters and converter service new possible distance objects could use with ease convert methods and no need to create another method in Interface and in its implementations. Also, I believe in a sense of single responsibility convert methods should be managed with convert service.
- I know why getters of Distance instances were private, but I suggest making them public to get the possibility for retrieving them from other objects.
- I changed float types of variables to double as I believe it correlates with best practices due to float restrictions.
- Still not sure that calculates are correct. Please, check different values in tests and create a task if you agree with me.
Will be waiting for your opinion on the mentioned suggestions.
Hi @EEM86 thank you for your suggestion and patience. I had a busy week. I agree with you that is kind of violating the SRP, but implementing a converter for every unit it results in the same amount of code being implemented for new units.
I discussed the idea with the colleague and he is suggesting that we should implement a SI system: https://github.com/iTitus/commons/commit/0a1828dc01f3be1ee5f262100c9b346b77080b30 I kind of like that idea as it would remove a lot of constraints and converting units we basically by its self. So maybe we should wait a bit and see if the idea of @iTitus would be an improvement.
I got most of it set up now over at https://github.com/iTitus/commons/tree/master/src/main/java/io/github/ititus/si I will add a few missing units and maybe tweak the value type and then prototype a PR.
The code seems to be too coupled. It will be hard to extend this code when you add new distance items. Also, I refactored conversion formulas. It is better to create a new class for that, but for now, I think it is ok to have public vars in the interface. And the most important - using float and double for formulas is not a good idea. The calculations aren't quite correct.
In the scope of this task seem all is done.