#Can somebody review my code for a calculator and see how it could be written better?

62 messages · Page 1 of 1 (latest)

limpid pendant
#

This is my very first independant rust project that I made by myself. I am very curious on the things i could've done better and how i couldve wrote everything more efficiently / better looking.

use std::io;

fn main() {
   let _operators: [char; 4] = ['+', '-', '*', '/'];

   let num1: f32;
   let num2: f32;
   let result: f32;
   let operator: String;

   loop {
      let mut input = String::new();
      println!("enter num1:");
      io::stdin().read_line(&mut input).expect("failed to read line.");

      match input.trim().parse() {
         Ok(n) => {
            num1 = n; break;
         },
         Err(_) => continue,
      }
   };

   loop {
      let mut input = String::new();
      println!("enter num2:");
      io::stdin().read_line(&mut input).expect("failed to read line.");

      match input.trim().parse() {
         Ok(n) => {
            num2 = n; break;
         },
         Err(_) => continue,
      }
   };

   loop {
      let mut input = String::new();
      println!("enter an operator (+, -, *, /):");
      io::stdin().read_line(&mut input).expect("failed to read line.");

      match input.trim() {
         "+" | "-" | "*" | "/" => {
            operator = input.trim().to_string();
            break;
         },
         _ => continue
      }
   }

   match operator.as_str() {
      "+" => result = num1 + num2,
      "-" => result = num1 - num2,
      "*" => result = num1 * num2,
      "/" => result = num1 / num2,
      _ => return
   }

   println!("{num1} + {num2} = {result}");
}```

thanks!
mellow knot
#

It's good! Pretty easy to follow.

The match statements are using "break" and "continue" but they're not necessary here. Specifically, "break" is actually doing nothing and could be removed. Consider replacing "continue" with {}, which is a "do nothing" command.

If you want to go more functional, consider breaking things up into functions. You ask for input twice; why not make a function that asks the user for input and returns the input given?

upbeat flicker
#

Functionality wise, you don't actually print the operation used, you always print +

limpid pendant
#

and the function idea is smart i dont know how i didnt think of that

#

very tired aha

limpid pendant
#

one thing i was wondering

upbeat flicker
limpid pendant
mellow knot
#

Oh mb, they're breaking a loop. You're right

limpid pendant
#

one thing i wanted to ask

#

the operator variable

#

i am constantly calling different to_string and as_str functions on it

#

is there any better way about going about that

mellow knot
#

You take the input, and turn it into a string. This is necessary because you need to store it

#

Then you compare it to strs so you change it back

#

Honestly a pretty clean way to do it imo

limpid pendant
#

really?

#

very happy to hear that lmao

#

this is the first thing

#

i ever written

#

thanks a lot for reviewing it

mellow knot
#

Are you coming from C or anything like that?

limpid pendant
#

c++ a very long time ago

#

my recent strong focus was on luau

upbeat flicker
#

In rust you can assign from any block, so it'd be better in my option to return the value from the loop, rather than assigning and breaking. The break keyword is equivalent to return but for loops.

operator = loop {
    let mut input = String::new();
    println!("enter an operator (+, -, *, /):");
    io::stdin()
        .read_line(&mut input)
        .expect("failed to read line.");

    match input.trim() {
        "+" | "-" | "*" | "/" => {
            break input.trim().to_string();
        }
        _ => continue,
    }
};```
limpid pendant
limpid pendant
#

but i failed to do it

upbeat flicker
#

You can also get rid of the initial creation of the operator variable before it's necessary

limpid pendant
#

for some reason

limpid pendant
#

instead of grouping the variables together

#

you want me to declare them when i actually need to use them?

upbeat flicker
#

Makes sense to me, why have your variable declarations so far from where you're actually using them, that's more personal preference though

limpid pendant
#

most likely would look nicer

upbeat flicker
#

You can also bind a matched pattern to a variable in the operator loop like so:

match input.trim() {
    op_str @ ("+" | "-" | "*" | "/") => {
        break op_str.to_string();
    }
    _ => continue,
}```
to avoid calling to trim multiple times
limpid pendant
#

gonna search this up

upbeat flicker
#

Chapter 6 and 18 of the Rust Book has a ton of information about what you can do with pattern matching

#

It's pretty much the only use for the @ binding, however binding withing a pattern alone is quite powerful

#

You can of course do similar for result, to get rid of all those extra result assignments and just have 1:

let result = match operator.as_str() {
    "+" => num1 + num2,
    "-" => num1 - num2,
    "*" => num1 * num2,
    "/" => num1 / num2,
    _ => return,
};```
limpid pendant
#

that's smart

upbeat flicker
#

By the way, personally I would swap the return for a panic!(), since it should be impossible for you to reach that branch

limpid pendant
#

man

#

getting your code reviewed by somebody else is actually so beneficial

upbeat flicker
#

Also, you might like to learn that you can use pattern matching in if statements, not just match, for example:

match input.trim().parse() {
    Ok(n) => {
        break n;
    }
    Err(_) => continue,
}```
in your num loops can be replaced with
```rust
if let Ok(n) = input.trim().parse() {
    break n;
}```
Since we already discussed the continues are unnecessary.
#

So, once you split off your num read loops into their own functions, I think the next step you could take is moving your operations into an Enum instead of hard coding the string matches in 2 separate places.

This would let you get rid of the .to_string() and .as_str() calls, and instead .parse() the strings like you do for your floats, though this might be a bit above your current level so feel free to come back for more help if you get stuck.

limpid pendant
#

someone mentioned it the other day

#

something similar

#

i cannot figure out what that means

upbeat flicker
#

That's fair, these two are equivalent if that helps

match value {
    pattern => {
        if_block
    }
    _ => {
        else_block
    }
}

if let pattern = value {
    if_block
} else {
    else_block
}```
but it's basically just the `let` keyword signifies you want to use pattern matching instead of regular boolean expressions
#

Overall, the if let syntax is better if you're making one or few comparisons, and don't want to cover all cases, while match is better when you want to ensure all cases will be covered.

limpid pendant
#

i will look at this tmrw and make sense of it lmao

#

it’s 1 am

upbeat flicker
#

No worries, it doesn't read quite the same as a match statement so it can be a little confusing