Formatting and proper comparision

src/num.rs

@@ -257,6 +257,7 @@ mod tests {
 
     #[test]
     fn test_gcd() {
+        assert_eq!(u8::gcd(2, 3), 1);
         assert_eq!(u8::gcd(2, 2), 2);
         assert_eq!(u8::gcd(2, 8), 2);
         assert_eq!(u16::gcd(36, 48), 12);
@@ -264,6 +265,8 @@ mod tests {
 
     #[test]
     fn test_lcm() {
+        assert_eq!(u32::lcm(2, 8), 8);
+        assert_eq!(u16::lcm(2, 3), 6);
         assert_eq!(usize::lcm(15, 30), 30);
         assert_eq!(u128::lcm(1, 5), 5);
     }

src/rational.rs

@@ -32,10 +32,10 @@ macro_rules! frac {
         frac!($w) + frac!($n / $d)
     };
     ($n:literal / $d:literal) => {
-        frac($n, $d)
+        $crate::rational::Frac::new($n, $d)
     };
     ($w:literal) => {
-        frac($w, 1)
+        $crate::rational::Frac::new($w, 1)
     };
 }
 
@@ -45,35 +45,44 @@ enum FracOp {
     Other,
 }
 
-/// Create a new rational number from signed or unsigned integers
-#[allow(dead_code)]
-fn frac<T: Int + Int<Un = U>, U: Unsigned>(n: T, d: T) -> Frac<U> {
-    let mut sign = Sign::Positive;
-
-    if n.is_neg() {
-        sign = !sign;
-    }
-
-    if d.is_neg() {
-        sign = !sign;
-    }
-
-    let numer = n.to_unsigned();
-    let denom = d.to_unsigned();
-
-    Frac::new(numer, denom, sign)
-}
-
 impl<T: Unsigned> Frac<T> {
-    /// Create a new rational number from unsigned integers and a sign
+    /// Create a new rational number from signed or unsigned arguments
     ///
     /// Generally, you will probably prefer to use the [frac!](../macro.frac.html) macro
-    /// instead, as that accepts both signed and unsigned arguments
-    pub fn new(n: T, d: T, s: Sign) -> Frac<T> {
-        Self::new_raw(n, d, s).reduce()
+    /// instead, as that is easier for mixed fractions and whole numbers
+    pub fn new<N: Int + Int<Un = T>>(n: N, d: N) -> Frac<T> {
+        Self::new_unreduced(n, d).reduce()
     }
 
-    fn new_raw(n: T, d: T, s: Sign) -> Frac<T> {
+    /// Create a new rational number from signed or unsigned arguments
+    /// where the resulting fraction is not reduced
+    pub fn new_unreduced<N: Int + Int<Un = T>>(n: N, d: N) -> Frac<T> {
+        if d.is_zero() {
+            panic!("Fraction can not have a zero denominator");
+        }
+
+        let mut sign = Sign::Positive;
+
+        if n.is_neg() {
+            sign = !sign;
+        }
+
+        if d.is_neg() {
+            sign = !sign;
+        }
+
+        let numer = n.to_unsigned();
+        let denom = d.to_unsigned();
+
+        Frac {
+            numer,
+            denom,
+            sign,
+        }
+    }
+
+    /// Create a new rational from the raw parts
+    fn raw(n: T, d: T, s: Sign) -> Frac<T> {
         if d.is_zero() {
             panic!("Fraction can not have a zero denominator");
         }
@@ -82,7 +91,7 @@ impl<T: Unsigned> Frac<T> {
             numer: n,
             denom: d,
             sign: s,
-        }
+        }.reduce()
     }
 
     /// Determine the output sign given the two input signs
@@ -108,51 +117,63 @@ impl<T: Unsigned> Frac<T> {
     }
 }
 
-impl<T: Unsigned + Add<Output = T> + Sub<Output = T> + Mul<Output = T> + Div<Output = T>> PartialOrd
-    for Frac<T>
+impl<T> PartialOrd for Frac<T>
+where
+    T: Unsigned + Add<Output = T> + Sub<Output = T> + Mul<Output = T> + Div<Output = T>,
 {
     fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
         Some(self.cmp(other))
     }
 }
 
-impl<T: Unsigned + Add<Output = T> + Sub<Output = T> + Mul<Output = T> + Div<Output = T>> Ord
-    for Frac<T>
+impl<T> Ord for Frac<T>
+where
+    T: Unsigned + Add<Output = T> + Sub<Output = T> + Mul<Output = T> + Div<Output = T>,
 {
     fn cmp(&self, other: &Self) -> Ordering {
-        if self.sign == other.sign {
-            if self.denom == other.denom {
-                return self.numer.cmp(&other.numer);
+        if self.sign != other.sign {
+            return if self.sign == Sign::Positive {
+                Ordering::Greater
             } else {
-                // @TODO fix logic for comparing fractions of different denominators
-                let gcd = T::gcd(self.denom, other.denom);
-                let x = gcd / self.denom;
-                let y = gcd / self.denom;
-                let mut a = self.clone();
-                let mut b = other.clone();
-
-                a.numer = a.numer * x;
-                a.denom = a.denom * x;
-
-                b.numer = b.numer * y;
-                b.denom = b.denom * y;
-
-                assert_eq!(a.denom, b.denom, "Denominators should be equal here. {:#?}{:#?}{:?}{:?}", a, b, x, y);
-
-                a.numer.cmp(&b.numer)
-            }
-        } else {
-            if self.sign == Sign::Positive {
-                return Ordering::Greater;
-            } else {
-                return Ordering::Less;
-            }
+                Ordering::Less
+            };
         }
+
+        if self.denom == other.denom {
+            return self.numer.cmp(&other.numer);
+        }
+
+        let mut a = self.reduce();
+        let mut b = other.reduce();
+
+        if a.denom == b.denom {
+            assert!(false, "{:#?}\n{:#?}", a, b);
+            return a.numer.cmp(&b.numer);
+        }
+
+        let lcm = T::lcm(self.denom, other.denom);
+        let x = lcm / self.denom;
+        let y = lcm / other.denom;
+
+        a.numer *= x;
+        a.denom *= x;
+
+        b.numer *= y;
+        b.denom *= y;
+
+        debug_assert_eq!(
+            a.denom, b.denom,
+            "Denominators should be equal here. \n{:#?}\n{:#?}\n{:?}\n{:?}",
+            a, b, x, y
+        );
+
+        a.numer.cmp(&b.numer)
     }
 }
 
-impl<T: Unsigned + Add<Output = T> + Sub<Output = T> + Mul<Output = T> + Div<Output = T>> Mul
-    for Frac<T>
+impl<T> Mul for Frac<T>
+where
+    T: Unsigned + Add<Output = T> + Sub<Output = T> + Mul<Output = T> + Div<Output = T>,
 {
     type Output = Self;
 
@@ -161,20 +182,22 @@ impl<T: Unsigned + Add<Output = T> + Sub<Output = T> + Mul<Output = T> + Div<Out
         let denom = self.denom * rhs.denom;
         let sign = Self::get_sign(self, rhs, FracOp::Other);
 
-        Self::new(numer, denom, sign)
+        Self::raw(numer, denom, sign)
     }
 }
 
-impl<T: Unsigned + Add<Output = T> + Sub<Output = T> + Mul<Output = T> + Div<Output = T>> MulAssign
-    for Frac<T>
+impl<T> MulAssign for Frac<T>
+where
+    T: Unsigned + Add<Output = T> + Sub<Output = T> + Mul<Output = T> + Div<Output = T>,
 {
     fn mul_assign(&mut self, rhs: Self) {
         *self = self.clone() * rhs
     }
 }
 
-impl<T: Unsigned + Add<Output = T> + Sub<Output = T> + Mul<Output = T> + Div<Output = T>> Div
-    for Frac<T>
+impl<T> Div for Frac<T>
+where
+    T: Unsigned + Add<Output = T> + Sub<Output = T> + Mul<Output = T> + Div<Output = T>,
 {
     type Output = Self;
 
@@ -183,20 +206,22 @@ impl<T: Unsigned + Add<Output = T> + Sub<Output = T> + Mul<Output = T> + Div<Out
         let denom = self.denom * rhs.numer;
         let sign = Self::get_sign(self, rhs, FracOp::Other);
 
-        Self::new(numer, denom, sign)
+        Self::raw(numer, denom, sign)
     }
 }
 
-impl<T: Unsigned + Add<Output = T> + Sub<Output = T> + Mul<Output = T> + Div<Output = T>> DivAssign
-    for Frac<T>
+impl<T> DivAssign for Frac<T>
+where
+    T: Unsigned + Add<Output = T> + Sub<Output = T> + Mul<Output = T> + Div<Output = T>,
 {
     fn div_assign(&mut self, rhs: Self) {
         *self = self.clone() / rhs
     }
 }
 
-impl<T: Unsigned + Add<Output = T> + Sub<Output = T> + Mul<Output = T> + Div<Output = T>> Add
-    for Frac<T>
+impl<T> Add for Frac<T>
+where
+    T: Unsigned + Add<Output = T> + Sub<Output = T> + Mul<Output = T> + Div<Output = T>,
 {
     type Output = Self;
 
@@ -222,27 +247,29 @@ impl<T: Unsigned + Add<Output = T> + Sub<Output = T> + Mul<Output = T> + Div<Out
             let denom = a.denom * b.denom;
             let sign = Self::get_sign(a, b, FracOp::Other);
 
-            return Self::new(numer, denom, sign);
+            return Self::raw(numer, denom, sign);
         }
 
         let numer = a.numer + b.numer;
         let denom = self.denom;
         let sign = Self::get_sign(a, b, FracOp::Other);
 
-        Self::new(numer, denom, sign)
+        Self::raw(numer, denom, sign)
     }
 }
 
-impl<T: Unsigned + Add<Output = T> + Sub<Output = T> + Mul<Output = T> + Div<Output = T>> AddAssign
-    for Frac<T>
+impl<T> AddAssign for Frac<T>
+where
+    T: Unsigned + Add<Output = T> + Sub<Output = T> + Mul<Output = T> + Div<Output = T>,
 {
     fn add_assign(&mut self, rhs: Self) {
         *self = self.clone() + rhs
     }
 }
 
-impl<T: Unsigned + Add<Output = T> + Sub<Output = T> + Mul<Output = T> + Div<Output = T>> Sub
-    for Frac<T>
+impl<T> Sub for Frac<T>
+where
+    T: Unsigned + Add<Output = T> + Sub<Output = T> + Mul<Output = T> + Div<Output = T>,
 {
     type Output = Self;
 
@@ -257,19 +284,20 @@ impl<T: Unsigned + Add<Output = T> + Sub<Output = T> + Mul<Output = T> + Div<Out
             let denom = a.denom * b.denom;
             let sign = Self::get_sign(a, b, FracOp::Subtraction);
 
-            return Self::new(numer, denom, sign);
+            return Self::raw(numer, denom, sign);
         }
 
         let numer = a.numer - b.numer;
         let denom = a.denom;
         let sign = Self::get_sign(a, b, FracOp::Subtraction);
 
-        Self::new(numer, denom, sign)
+        Self::raw(numer, denom, sign)
     }
 }
 
-impl<T: Unsigned + Add<Output = T> + Sub<Output = T> + Mul<Output = T> + Div<Output = T>> SubAssign
-    for Frac<T>
+impl<T> SubAssign for Frac<T>
+where
+    T: Unsigned + Add<Output = T> + Sub<Output = T> + Mul<Output = T> + Div<Output = T>,
 {
     fn sub_assign(&mut self, rhs: Self) {
         *self = self.clone() - rhs
@@ -317,7 +345,10 @@ mod tests {
 
     #[test]
     fn cmp_test() {
-        // assert_eq!(Ordering::Equal, Frac::new_raw(1u8, 2u8, Sign::Positive).cmp(&Frac::new_raw(4u8, 8u8, Sign::Positive)));
+        assert!(-frac!(5 / 3) < frac!(1 / 10_000));
+        assert!(frac!(1 / 10_000) > -frac!(10));
+        assert!(frac!(1 / 3) < frac!(1 / 2));
+        assert_eq!(frac!(1 / 2), frac!(3 / 6));
     }
 
     #[test]