2
\$\begingroup\$

A \$Z^m\$ number system includes integers in the interval \$[0, m)\$ when \$m > 0\$ or \$(m, 0]\$ when \$m < 0\$. The code defines a trait Mod to represent numbers in this system and the +, -, and * operator for it. 

use std::ops::Add;
use std::ops::Mul;
use std::ops::Sub;
use std::ops::Rem;


struct Mod<T>
    where T: Modulo<T> + Mul<Output=T> + Sub<Output=T> + Add<Output=T> + Rem<Output=T> + Copy + Clone
{
    modulo: T,
    i: T,
}

trait Modulo<T>
    where T: Add<Output=T> + Rem<Output=T> + Copy + Clone
{
    fn modulo(self, n: T) -> T;
}

impl<T> Modulo<T> for T 
    where T: Add<Output=T> + Rem<Output=T> + Copy + Clone
{
    fn modulo(self, n: T) -> T {
        ((self % n) + n) % n
    }
}

impl<T> Mod<T> 
    where T: Modulo<T> + Mul<Output=T> + Sub<Output=T> + Add<Output=T> + Rem<Output=T> + Copy + Clone
{
    fn new(modulo: T, i: T) -> Mod<T> {
        let n = i.modulo(modulo);
        Mod {
            modulo: modulo,
            i: n,
        }
    }
}

impl<T> Add for Mod<T> 
    where T: Modulo<T> + Mul<Output=T> + Sub<Output=T> + Add<Output=T> + Rem<Output=T> + Copy + Clone
{
    type Output = Mod<T>;
    fn add(self, other: Mod<T>) -> Mod<T> {
        Mod::new(self.modulo, self.i + other.i)
    }
}

impl<T> Sub for Mod<T> 
    where T: Modulo<T> + Mul<Output=T> + Sub<Output=T> + Add<Output=T> + Rem<Output=T> + Copy + Clone
{
    type Output = Mod<T>;
    fn sub(self, other: Mod<T>) -> Mod<T> {
        Mod::new(self.modulo, self.i - other.i)
    }
}

impl<T> Mul for Mod<T> 
    where T: Modulo<T> + Mul<Output=T> + Sub<Output=T> + Add<Output=T> + Rem<Output=T> + Copy + Clone
{
    type Output = Mod<T>;
    fn mul(self, other: Mod<T>) -> Mod<T> {
        Mod::new(self.modulo, self.i * other.i)
    }
}



fn main() {
    let x = Mod::new(-5i8, 3i8);
    let y = Mod::new(-5i8, 8i8);
    println!("{}", (x + y).i);
    let x = Mod::new(-5i8, 3i8);
    let y = Mod::new(-5i8, 8i8);
    println!("{}", (x - y).i);
    let x = Mod::new(-5i8, 3i8);
    let y = Mod::new(-5i8, 8i8);
    println!("{}", (x * y).i);
    let x = Mod::new(-5i16, 3i16);
    let y = Mod::new(-5i16, 8i16);
    println!("{}", (x + y).i);
    let x = Mod::new(-5i16, 3i16);
    let y = Mod::new(-5i16, 8i16);
    println!("{}", (x - y).i);
    let x = Mod::new(-5i16, 3i16);
    let y = Mod::new(-5i16, 8i16);
    println!("{}", (x * y).i);
    let x = Mod::new(-5, 3);
    let y = Mod::new(-5, 8);
    println!("{}", (x + y).i);
    let x = Mod::new(-5, 3);
    let y = Mod::new(-5, 8);
    println!("{}", (x - y).i);
    let x = Mod::new(-5, 3);
    let y = Mod::new(-5, 8);
    println!("{}", (x * y).i);
    let x = Mod::new(5u8, 3u8);
    let y = Mod::new(5u8, 8u8);
    println!("{}", (x + y).i);
    let x = Mod::new(5u8, 3u8);
    let y = Mod::new(5u8, 8u8);
    println!("{}", (x - y).i);
    let x = Mod::new(5u8, 3u8);
    let y = Mod::new(5u8, 8u8);
    println!("{}", (x * y).i);
}

I tried to achieve the goals laid out in the previous version:

  1. Changed the modulo method for better performance.
  2. Generalized over a bunch of types other than i32.

All suggestions are still welcome. In particular, requiring T to implement Copy and Clone might be a bit too restrictive, I would like to relax that.

Improve this question
\$\endgroup\$

1 Answer 1

Reset to default
1
\$\begingroup\$

  1. Place multiple imports from the same module on one line.

  2. I would heartily recommend against interleaving a trait definition (Modulo) and a struct definition (Mod).

  3. I prefer to not put trait bounds on the trait definition. Those usually want to be a supertrait or just on a trait implementation.

  4. Similarly, I prefer not put trait bounds on a struct definition; just on on implementation.

  5. There's no need for the Clone bound — it's never used.

  6. I'd make your new "operator" trait match others: Parameterize the right-hand side (Rhs) and the Output as an associated type.

  7. Then you can DRY up your duplicated trait bounds by creating a new trait that has all the other traits as a supertrait.

  8. While you are at it, remove the type parameter T because you always use it in the same fashion; it might as well be hardcoded.

  9. Place a space around = when restricting associated types in traits.

  10. There's no need for the temp var in the Mod constructor.

  11. Tests. TESTS. TESTS You are so close to already having tests! Your main function can be basically transformed into a sequence of tests. Then the computer can verify them, instead of having to read the output each time. Please give them useful names that describe what each tests, not my stupid sequential names.

use std::ops::{Add, Mul, Sub, Rem};

trait Modulo<Rhs = Self> {
    type Output;

    fn modulo(self, n: Rhs) -> Self::Output;
}

impl<T> Modulo<T> for T
    where T: Add<Output = T> + Rem<Output = T> + Copy
{
    type Output = T;

    fn modulo(self, n: T) -> T {
        ((self % n) + n) % n
    }
}


trait ZMod: Modulo<Output = Self> + Mul<Output = Self> + Sub<Output = Self> + Add<Output = Self> + Rem<Output = Self> + Copy {}
impl<T> ZMod for T
    where T: Modulo<Output = T> + Mul<Output = T> + Sub<Output = T> + Add<Output = T> + Rem<Output = T> + Copy
{}


struct Mod<T> {
    modulo: T,
    i: T,
}

impl<T> Mod<T>
    where T: ZMod
{
    fn new(modulo: T, i: T) -> Mod<T> {
        Mod {
            modulo: modulo,
            i: i.modulo(modulo),
        }
    }
}

impl<T> Add for Mod<T>
    where T: ZMod
{
    type Output = Mod<T>;
    fn add(self, other: Mod<T>) -> Mod<T> {
        Mod::new(self.modulo, self.i + other.i)
    }
}

impl<T> Sub for Mod<T>
    where T: ZMod
{
    type Output = Mod<T>;
    fn sub(self, other: Mod<T>) -> Mod<T> {
        Mod::new(self.modulo, self.i - other.i)
    }
}

impl<T> Mul for Mod<T>
    where T: ZMod
{
    type Output = Mod<T>;
    fn mul(self, other: Mod<T>) -> Mod<T> {
        Mod::new(self.modulo, self.i * other.i)
    }
}

#[test]
fn t0() {
    let x = Mod::new(-5i8, 3i8);
    let y = Mod::new(-5i8, 8i8);
    assert_eq!(-4, (x + y).i);
}

#[test]
fn t1() {
    let x = Mod::new(-5i8, 3i8);
    let y = Mod::new(-5i8, 8i8);
    assert_eq!(-4, (x + y).i);
}


#[test]
fn t2() {
    let x = Mod::new(-5i8, 3i8);
    let y = Mod::new(-5i8, 8i8);
    assert_eq!(0, (x - y).i);
}

#[test]
fn t3() {
    let x = Mod::new(-5i8, 3i8);
    let y = Mod::new(-5i8, 8i8);
    assert_eq!(-1, (x * y).i);
}

#[test]
fn t4() {
    let x = Mod::new(-5i16, 3i16);
    let y = Mod::new(-5i16, 8i16);
    assert_eq!(-4, (x + y).i);
}

#[test]
fn t5() {
    let x = Mod::new(-5i16, 3i16);
    let y = Mod::new(-5i16, 8i16);
    assert_eq!(0, (x - y).i);
}

#[test]
fn t6() {
    let x = Mod::new(-5i16, 3i16);
    let y = Mod::new(-5i16, 8i16);
    assert_eq!(-1, (x * y).i);
}

#[test]
fn t7() {
    let x = Mod::new(-5, 3);
    let y = Mod::new(-5, 8);
    assert_eq!(-4, (x + y).i);
}

#[test]
fn t8() {
    let x = Mod::new(-5, 3);
    let y = Mod::new(-5, 8);
    assert_eq!(0, (x - y).i);
}

#[test]
fn t9() {
    let x = Mod::new(-5, 3);
    let y = Mod::new(-5, 8);
    assert_eq!(-1, (x * y).i);
}

#[test]
fn t10() {
    let x = Mod::new(5u8, 3u8);
    let y = Mod::new(5u8, 8u8);
    assert_eq!(1, (x + y).i);
}

#[test]
fn t11() {
    let x = Mod::new(5u8, 3u8);
    let y = Mod::new(5u8, 8u8);
    assert_eq!(0, (x - y).i);
}

#[test]
fn t12() {
    let x = Mod::new(5u8, 3u8);
    let y = Mod::new(5u8, 8u8);
    assert_eq!(4, (x * y).i);
}
Improve this answer
\$\endgroup\$
1
  • \$\begingroup\$ Great, the code looks much better now. \$\endgroup\$
    – qed
    Sep 16, 2016 at 18:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.