#Review a rust newbie's code for a tiny fractions library

52 messages · Page 1 of 1 (latest)

real rivet
#

main.rs

mod fractions;
mod test;

use fractions::Fraction;

fn main() {
    let half = Fraction::new(1, 2).unwrap();
    let quarter = Fraction::new(1, 4).unwrap();

    let three_quarters = half + quarter;

    let three = three_quarters / quarter;

    println!("Half is: {half}");
    println!("Quarter is: {quarter}");
    println!("Three quarters is: {three_quarters}");
    println!("Three is: {three}");
}

It's pretty straight forwards. Looking for some criticism and maybe imrpovement suggestions. Yes, I'm aware there are libraries like num, this is mainly for the purposes of learning. Thank you for your time!

📎 fractions.rs 📎 test.rs
woven lichen
#

you can do that 2 ways: by accepting a std::num::NonZero in the denominator field or manually checking and panicking in new

#

and you can provide a try_new function that returns an option if there's a possibility of the denominator being 0

#

what this accomplishes is that when you know that a denominator cant be 0, you avoid the user API being flooded with unwraps

# 
let my_fraction = Fraction::new(1, 5);

however this function can panic at runtime and additional error handling is required when dealing with numbers coming from external sources:

let my_fraction = Fraction::try_new(1, 0);
assert!(my_fraction.is_err());
real rivet
#

i see, thank you

#

that's neat

real rivet
#

I've made some changes,
created checked_new, checked_simplify, checked... yada yada

but now I have a problem.
Should I make the operations like Fraction1 + Fraction2 need to be .unwrap()ed?

#

they can now fail, as there isn't an unwrap() inside

real rivet
#

let x = 1 / 0; //has a compile time lsp check, but let's me compile

error: this operation will panic at runtime
--> src\main.rs:25:13
|
25 | let x = 1 / 0;
| ^^^^^ attempt to divide 1_i32 by zero
|
= note: #[deny(unconditional_panic)] on by default

how do i make unconditional_panic message for my fraction?

real rivet
#

something akin to

image.png
woven lichen
#

hmm, never done one myself so you're on your own here

#

try asking google

real rivet
#

I've tried searching, but I'm getting lost lol

woven lichen
#

can you share the code with your recent changes?

real rivet
#

yeah

#

gimme a sec

woven lichen
#

why do you have an iterative approach to gcd? can you justify this?

# 
let max_iterations = 1000;
let mut iterations = 0;

while b != 0 {
    if iterations >= max_iterations {
        return Err(FractionError::GcdError);
    }

    let temp = b;
    b = a % b;
    a = temp;

    iterations += 1;
}
#

if rs let temp = b; b = a % b; a = temp; reaches the theoretical limit of how precise a number can be in terms of i64, i'd recommend finding a more optimal max_iterations limit

real rivet
#

instead of having a pub static ZERO, you can namespace zero under Fraction:
noted
why do you have an iterative approach to gcd? can you justify this
we're doing a lot of iteration in school atm, so I just went with what was the first idea (lisp✌️ )

woven lichen
# 
// Utility functions
pub fn checked_gcd
pub const fn gcd
pub fn checked_lcm
pub fn lcm

if these are utility functions, then consider making them private

#

pub is for what you expose to the end user interface

#

also, as far as i am concerned, fractions only deal with 1 sign

#

so adding an additional sign field and changing other fields to u32's will definitely extend the domain of your fractions

#

but it will cause 2 things: the Fraction struct will no longer be 8-byte aligned (rust may introduce padding to fix this), and every fraction will take an additional byte of sign data

#

in order to avoid both of them you can just store the sign in the denominator

#

so nom: u32, denom: i32

real rivet
#

I rewrote a lot of it now

#

I decided to try making it use u64

#

and sign is just a enum

#

pushed the changes, updated the tests

#

I'd like to know if you have any more improvements

woven lichen
#

if you're not concerned about squeezing that 33% performance improvement having a sign as an enum definitely wont hurt, however, it will harm the alignment. you can check with align_of::<YourType>()

#

it will most likely be 256 (64+64+8 rounded up to the nearest power of 2)

#

also, you may want to add more QoL end-user api to your struct

#

like Fraction::is_integer which checks whether the denominator is 1, stuff like that

#

stuff that dont necessarily implement stuff but imply intent better

#

something else, in

impl Add for Fraction {
    type Output = Self;

    fn add(self, other: Self) -> Self {
        let lcm = lcm(self.denominator, other.denominator);
        let numerator_self =
            self.sign_factor() * (self.numerator as i64) * (lcm / self.denominator) as i64;
        let numerator_other =
            other.sign_factor() * (other.numerator as i64) * (lcm / other.denominator) as i64;
        let numerator = numerator_self + numerator_other;

        Fraction::new(numerator, lcm as i64)
    }
}

casting u64 as i64 will lose precision, opt to avoid as
as your nom/denom are now unsigned, you can directly match the sign of the fractions and do operations based on that

match other.sign {
  Neg => // self - other implementation, no need for `sign_factor as i64 *` since both types are the same
  Pos => // self + other implementation
}```
wide onyx
#

allowing the denominator to be zero allows you to have the fun quirks of floats but in normal fractions

#

PartialEq should not be derived and instead implemented as self.numerator * other.denominator == other.numerator * self.denominator after factoring sign

#

if you want rational equality

#

oh, you call simplify in new()

#

honestly not my cup of tea having it not be lazily simplified

#

personally, i would simply store the numerator as an i64 and get rid of the extra explicitly stored sign entirely

real rivet
real rivet
wide onyx
wide onyx