Simplify Fraction and improve Rational. #476
Conversation
| pub trait Ratio<U> { | ||
| fn parts(&self) -> U; | ||
| fn total(&self) -> U; | ||
| #[derive(Debug, Serialize, Deserialize, JsonSchema)] |
|
|
||
| pub fn total(&self) -> U { | ||
| self.total | ||
| } |
There was a problem hiding this comment.
struct Rational<U> deserves its own module
| pub trait CheckedMul<Rhs = Self> { | ||
| type Output; | ||
|
|
||
| fn checked_mul(self, rhs: Rhs) -> Option<Self::Output>; | ||
| } | ||
|
|
||
| impl<U, T> Fraction<U> for Rational<T> | ||
| pub trait CheckedAdd<Rhs = Self> { | ||
| type Output; | ||
|
|
||
| fn checked_add(self, rhs: Rhs) -> Option<Self::Output>; | ||
| } | ||
|
|
There was a problem hiding this comment.
these two traits should be moved out into their own checked_ops module
| { | ||
| #[track_caller] | ||
| fn of<A>(&self, whole: A) -> A | ||
| // Multiplication of Rational > 1. |
There was a problem hiding this comment.
the comment is meaningless, please remove
| @@ -38,13 +36,10 @@ where | |||
| <D as TryInto<DIntermediate>>::Error: Debug, | |||
| DIntermediate: Into<T>, | |||
| D: Mul<D, Output = D> + Div<D, Output = D>, | |||
| U: Zero + PartialEq + Into<D>, | |||
| U: Zero + PartialEq + Into<D> + PartialOrd + Copy, | |||
There was a problem hiding this comment.
are you sure we need more trait bounds here
| fn checked_add(self, rhs: Self) -> Option<Self::Output> { | ||
| self.denominator | ||
| .checked_mul(rhs.denominator) | ||
| .and_then(|common_denominator| { | ||
| self.nominator | ||
| .checked_mul(rhs.denominator) | ||
| .and_then(|scaled_left_nom| { | ||
| rhs.nominator | ||
| .checked_mul(self.denominator) | ||
| .and_then(|scaled_right_nom| { | ||
| scaled_left_nom | ||
| .checked_add(scaled_right_nom) | ||
| .map(|nominator| Rational::new(nominator, common_denominator)) | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
the good news is that this is already implemented in Price and battle-tested, no need to do it again
| self.denominator | ||
| .checked_mul(rhs.denominator) | ||
| .and_then(|common_denominator| { |
There was a problem hiding this comment.
would overflow in many cases when it should not, for example: with U=u16, (1,u16::MAX) + (1,u16::MAX)
| impl From<Percent> for Rational<Units> { | ||
| fn from(percent: Percent) -> Self { | ||
| Self::new(percent.units(), Percent::HUNDRED.units()) |
There was a problem hiding this comment.
I believe we have discussed another conversion Percent -> Ratio
| impl TryFrom<Rational<Units>> for Percent { | ||
| type Error = Error; |
There was a problem hiding this comment.
not sure we need this conversion
| let lhs: Rational<Units> = self.into(); | ||
| let rhs: Rational<Units> = rhs.into(); | ||
|
|
||
| lhs.checked_add(rhs).and_then(|res| res.try_into().ok()) |
There was a problem hiding this comment.
it'd be trivial to implement it through Units::checked_add instead
| type Output = Self; | ||
|
|
||
| fn div(self, rhs: Self) -> Self::Output { | ||
| Percent(self.0 / rhs.0) |
There was a problem hiding this comment.
let's retain the control in our hands by checking the preconditions ourselves, add a debug_assert!(this != Zero)
| type Output = Self; | ||
|
|
||
| fn rem(self, rhs: Self) -> Self::Output { | ||
| Percent(self.0 % rhs.0) |
| .lossy_mul(&Rational::new(self.amount_quote, rhs.amount)) | ||
| .lossy_mul(&Ratio::new(self.amount_quote.into(), rhs.amount.into())) |
There was a problem hiding this comment.
no guarantees that the price is < 1 !!!
No description provided.