fix : Use of integer for prices, therefore also for weight #433
Conversation
rename confusing "from" and "to" to sender_country and recipient_country as suggested
|
Issue 432 #432 |
| } | ||
| } | ||
|
|
||
| fn is_international(&self) -> ??? { | ||
| // Something goes here... | ||
| } | ||
|
|
||
| fn get_fees(&self, cost_per_kg: f32) -> ??? { | ||
| // Something goes here... | ||
| fn get_fees(&self, cents_per_kg: i32) -> ??? { |
There was a problem hiding this comment.
Perhaps this should also be cents_per_gram. Otherwise, the output may have to be a float, which we're trying to avoid. For example, a package weighing 150 grams -- with a cost of 10 cents per kg -- would have a cost of 1.5 cents.
There was a problem hiding this comment.
1.5 cent will be rounded and that's what we want because we can't pay 0.5 cents anyway.
Cents per gram is not precise enough as a price is usually less than one cent per gram.
Yes it require to divide by 1000. I think that is not hard to grasp. I also think it would be nice to add a link to a good explanation why float are bad for money.
Like this: https://stackoverflow.com/questions/3730019/why-not-use-double-or-float-to-represent-currency
There was a problem hiding this comment.
Currently, it asks the user to supply the return type. Many users likely will reason that it should be a float, since otherwise information is lost (and as-is it is unclear whether they should round or truncate).
So, to avoid another issue, it might be good to either:
- Force the return type to be an integer, and add a comment asking the user to truncate the result. -or-
- Use
cents_per_gram, where we will not have to deal with fractions.
There was a problem hiding this comment.
There is no need to truncate or round, integers operations do it by themselves.
Relevant information is lost with floats, not with integer.
I think your option 1 is the good one, no need to ask for any truncate or rounding. It is done naturally by integers operation.
Option 2 makes no sense as even the smallest integer price for 1 gram is out of scale ($1000 for 1kg).
There was a problem hiding this comment.
Thinking through this as a student, I would notice that 1254 / 1000 == 1. I would be hesitant to lose the extra 254 grams via an integer division.
Since Rustlings did not specify how to handle excess grams, I would likely decide the safest approach is to pro-rate cents_per_kg, maybe like this:
fn get_fees(&self, cents_per_kg: i32) -> i32 {
let weight_in_kgs = (self.weight_in_grams as f32) / 1000.;
(weight_in_kgs * (cents_per_kg as f32)) as i32
}Yet even now, I am unsure whether I wrote the function as Rustlings intended -- should I have rounded or truncated the result? (Money is often rounded.)
So, this is why I recommend including a comment explaining exactly how the fees should be computed (or just making it easy with cents_per_kg). I am just trying to avoid another Issue later on 😀
There was a problem hiding this comment.
We could also simply store weight in kg and integer, ignoring grams for the example it makes everything more simple and is more correct than using floats for money.
Should I do another pull request this way ?
There was a problem hiding this comment.
😀 Yes, that is a much better method and (except with large numbers) yields the same result! So perhaps a specification would be something like:
// Fees are pro-rated. Disregard any final fractions of a cent.
// Try to compute the fees without using floats!There was a problem hiding this comment.
As you want as long there's no float used for money, I'm happy.
I just did the clippy1 exercice with the beautifull advice :
if y != x {
| ^^^^^^ help: consider comparing them within some error: `(y - x).abs() > error`
That's how floats must be handled !
There was a problem hiding this comment.
We could also simply store weight in kg and integer, ignoring grams for the example it makes everything more simple and is more correct than using floats for money. Should I do another pull request this way ?
It might be easiest just to make them all the same units, but either way is probably fine as long as it's explained how to compute the fees. Other than this, I think it looks great!
There was a problem hiding this comment.
I'm a bit unsure about the units puzzle: it risks losing focus, but a bit of non-Rust-related challenge makes the exercises more amenable. No strong opinion anyway.
| } | ||
| } | ||
|
|
||
| fn is_international(&self) -> ??? { | ||
| // Something goes here... | ||
| } | ||
|
|
||
| fn get_fees(&self, cost_per_kg: f32) -> ??? { | ||
| // Something goes here... | ||
| fn get_fees(&self, cents_per_kg: i32) -> ??? { |
There was a problem hiding this comment.
I'm a bit unsure about the units puzzle: it risks losing focus, but a bit of non-Rust-related challenge makes the exercises more amenable. No strong opinion anyway.
|
|
||
| let country_fee = ???; | ||
| let cents_per_kg = ???; |
There was a problem hiding this comment.
Random thought, totally out of scope for this PR: it feels weird to me that we have learners guess backwards the appropriate input to obtain a predetermined output.
Also, I'm starting to think that forcing miscompilations through invalid code like ??? is something we should dial back a bit.
fix : Use of integer for prices, therefore also for weight
fix : Use of integer for prices, therefore also for weight
rename confusing "from" and "to" to sender_country and recipient_country as suggested