5
\$\begingroup\$

As the title suggests, I'm learning rust. My go to starter project when learning a language is to write a calculator. It's relatively simple, but complex enough that you will need to utilize many features of a language.

This particular implementation only covers the base PEMDAS as well as pseudo handling the unary negative operator. Functions, Variables, and Unary operators are not implemented.

I have implemented a shunting yard algorithm for parsing the input into a vector of tokens. Then process the tokens left to right, updating the vector to to store intermediate results until the vector has a length of one and return an Option as the result. Returning None if any step of parsing and/or evaluating the expression fails.

Questions:

  • With this written as a library module, does it make sense to return an Option, or is it better to use a Result? Is there a rule of thumb of when to use one over the other?

  • I said I pseudo handle the unary negative operator. As it is implemented differently than the other operators. How could I better implement this operator? Subjective Questions/Advice

  • Did I fall victim to any common mistakes made by people learning this language?

  • I am looking to generally improve the level at which I program. Is there any general suggestions on other ways I improve my code quality?

Trivial main.rs

use std::{io::{self}, process::exit};
use calculator;

fn main() {
    loop {
        let mut user_input = String::new();
        io::stdin().read_line(&mut user_input).expect("Unable to read stdin");
        if user_input.trim().eq_ignore_ascii_case("quit") || user_input.trim().eq_ignore_ascii_case("q"){
            break;
        }
        //intentionally pass ownership of user_input since we don't want to use the raw input
        if let Some(result) = calculator::to_result(user_input) {
            println!("{:?}", result);
        }
    }
    exit(0);
}

lib.rs

#[derive(Debug)]
enum Token<T> {
    Number(T),
    Operator(OP),
}

#[derive(Debug,PartialEq)]
enum OpAssocation {
    LEFT,
    RIGHT,
}

#[derive(Debug,PartialEq)]
enum OPSymbol {
    ADD,
    SUB,
    MUL,
    DIV,
    EXP,
    LeftParen,
    RightParen,
}

impl OPSymbol {
    //Convert char to OP
    fn value(c: char) -> Option<OP> {
        match c {
            '+' => Some(OP { op_symbol: OPSymbol::ADD, precedence: 2, op_association: OpAssocation::LEFT }),
            '-' => Some(OP { op_symbol: OPSymbol::SUB, precedence: 2, op_association: OpAssocation::LEFT }),
            '*' => Some(OP { op_symbol: OPSymbol::MUL, precedence: 3, op_association: OpAssocation::LEFT }), 
            '/' => Some(OP { op_symbol: OPSymbol::DIV, precedence: 3, op_association: OpAssocation::LEFT }),
            '^' => Some(OP { op_symbol: OPSymbol::EXP, precedence: 4, op_association: OpAssocation::RIGHT }),
            '(' => Some(OP { op_symbol: OPSymbol::LeftParen, precedence: 0, op_association: OpAssocation::RIGHT }),
            ')' => Some(OP { op_symbol: OPSymbol::RightParen, precedence: 0, op_association: OpAssocation::RIGHT }),
            _ => None
        }
    }

    //evaluate OpSymbol and perform operation
    fn eval(i1:f64, i2:f64, op: &OPSymbol) -> Option<f64> {
        match op {
            OPSymbol::ADD => Some(i1+i2),
            OPSymbol::SUB => Some(i1-i2),
            OPSymbol::MUL => Some(i1*i2),
            OPSymbol::DIV => {
                if i2 == 0.0 {
                    println!("Can't divide by 0!");
                    return  None
                }else {
                    Some(i1/i2)
                }
            },
            OPSymbol::EXP => Some(i1.powf(i2)),
            _ => {
                println!("Can't evaluate invalid symbol");
                None
            }
        }
    }
}

#[derive(Debug)]
struct OP {
    op_symbol: OPSymbol,
    precedence: u8,
    op_association: OpAssocation,
}

/// Parses a given expression string and returns a result as an f64
/// If any parsing/calculation errors occur returns None
/// Uses the shunting yard algorithm to parse the inputs into reverse polish notation
/// Handles basic Addition, Subtraction, Multiplication, Division, Exponents, the unary - operator
/// ```
/// let result = match calculator::to_result(String::from("2+2")) {
///   Some(x) => x,
///   None => panic!("Test Failed")
/// };
/// assert_eq!(result, 4.0);
/// 
/// let result = match calculator::to_result(String::from("2*(1+3)^2")) {
///   Some(x) => x,
///   None => panic!("Test Failed")
/// };
/// assert_eq!(result, 32.0);
/// ```
pub fn to_result(input: String) -> Option<f64> {
    //immedieatly die if we find alpha characters
    if input.contains(|c: char| { c.is_ascii_alphabetic() }){ 
        println!("Found Invalid Input!");
        return None
    }
    
    //get rid of spaces
    //use map() and closure to combine steps?
    let mut trimmed_input = String::new();
    for s in input.split_whitespace() {
        trimmed_input.push_str(s);
    }
    //convert to iterator since String and &str can't be iterated on
    let char_iter = trimmed_input.chars().into_iter();
    
    //manage index and offset of &str
    let mut index = 0;
    let mut offset = 0;
    let mut prev_val = '`';
    //token vector that will be passed to obtain final f64 result
    let mut tokens: Vec<Token<f64>> = Vec::new();
    let mut op_stack: Vec<OP> = Vec::new();

    for c in char_iter {
        //handle unary negative operator
        //Better way to hanlde this?
        if (c.is_ascii_digit() || c == '.') || (index == 0 && c == '-') || (c == '-' && !prev_val.is_ascii_digit() && prev_val != '`') {
            index+=1;
            continue;
        } else {
            //found number boundry
            if offset != index {
                tokens.push(Token::Number(match trimmed_input[offset..index].parse() {
                    Ok(x) => x,
                    Err(_) => {
                        println!("Found Symbol, Expected Number");
                        return None
                    }
                }));
            }            
            //convert char to OpSymbol
            let op1 = match OPSymbol::value(c) {
                Some(x) => x,
                None => {
                    println!("Found invalid symbol");
                    return None
                }
            };
            //process logic for for op token
            //unconditionally add ( to the opstack
            if op1.op_symbol == OPSymbol::LeftParen {
                op_stack.push(op1);
            //hanlde )
            } else if op1.op_symbol == OPSymbol::RightParen && op_stack.len() > 0 {
                //pop opstack until ( is found
                while op_stack.len() > 0 {
                    let op2 = &op_stack[op_stack.len()-1];
                    if op2.op_symbol != OPSymbol::LeftParen {
                        tokens.push(Token::Operator(match op_stack.pop() {
                            Some(x) => x,
                            None => {
                                println!("Found invalider OP token");
                                return None
                            }
                        } ));
                    } else {
                        //pop the ( and leave the loop
                        op_stack.pop();
                        break;
                    }
                    //found a ) but no matching (
                    if op_stack.len() == 0 {
                        println!("Found mismatched ()");
                        return None
                    }
                }
            //handle other operators
            } else {
                //if op stack is empty just add op
                if op_stack.len() == 0 {
                    op_stack.push(op1);
                }else {
                    while op_stack.len() > 0 {
                        let op2 = &op_stack[op_stack.len()-1];
                        if op2.op_symbol != OPSymbol::LeftParen && (op2.precedence > op1.precedence ||(op1.precedence == op2.precedence && op1.op_association == OpAssocation::LEFT)){
                            tokens.push(Token::Operator(match op_stack.pop() {
                                Some(x) => x,
                                None => {
                                    println!("Failed to push operator to token output");
                                    return None
                                }
                            } ));
                        }else {
                            break;
                        }
                    }
                    op_stack.push(op1); 
                }
            }
        }
        prev_val = c;
        index+=1;
        offset = index;
    }
    //if end of input was not a ) or some other symbol offset to end of input must be a number. Push this number to output vector
    if offset < trimmed_input.len() {
        tokens.push(Token::Number(match trimmed_input[offset..].parse() {
            Ok(x) => x,
            Err(_) => {
                println!("Found Symbol, Expected Number");
                return None
            }

        }));
    }
    //finsished iterating over string push remaining op symbols on to output
    if op_stack.len() > 0 {
        while op_stack.len() > 0 {
            tokens.push(Token::Operator(match op_stack.pop() {
                Some(x) => {
                    if x.op_symbol == OPSymbol::LeftParen {
                        println!("Found unclosed (");
                        return None
                    }else {
                        x
                    }
                },
                None => continue
            } ));
        }
    }
    //missing symbol
    //probably breaks if unary symbols are ever implemented like !5
    if tokens.len() % 2 == 0 {
        println!("Invalid Expression");
        return None
    }
    get_result(tokens)
}

/// Convert vector of Tokens to a final result as a f64 number
fn get_result(mut tokens: Vec<Token<f64>>) -> Option<f64> {
    let mut index = 0;
    while tokens.len() > 1 {
        //temp result
        let mut r: Token<f64>= Token::Number(0.0) ;
        for t in &tokens {
            match t {
                Token::Number(_x) => {
                    index+=1;
                    continue;
                },
                Token::Operator(x) => {
                    let i1 = match tokens[index-2] {
                        Token::Number(n) => n,
                        _ => {
                            println!("Found OPSymbol, expected Number");
                            return  None;
                        }
                    };
                    let i2 = match tokens[index-1] {
                        Token::Number(n) => n,
                        _ => {
                            println!("Found OpSymbol, expected Number");
                            return None;
                        }
                    };
                    r = Token::Number(match OPSymbol::eval(i1, i2, &x.op_symbol) {
                        Some(x) => x,
                        _ => return None
                    });
                    break;
                    
                }
            }
            
        }//end for
        
        //can't borrow immutable and mutable in same scope so need to update tokens outside for loop
        //update tokens
        tokens.remove(index);
        tokens.remove(index -1);
        tokens.remove(index - 2);
        tokens.insert(index -2, r);
        //reset for next loop
        index =0;
    }//end while
    match tokens[0] {
        Token::Number(x) => Some(x),
        _ => {
            println!("Expected Number, found Symbol");
            return None
        }
    }
}

Link to github if it helps: https://github.com/ruinedme/rust-calculator/tree/main

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Looks nice! Since you asked for advice on Option versus Result, let’s start there.

Use Result to Pass Back Information about Recoverable Errors

You check for errors consistently. Right now, the error checks look like this:

if i2 == 0.0 {
    println!("Can't divide by 0!");
    return  None
}else {
    Some(i1/i2)
}

We see that, in this implementation, all errors print a string constant to standard output and short-circuit. This isn’t ideal, for a few reasons:

  • The functions have visible side-effects, which means you can’t perform optimizations like common-subexpression elimination or lazy evaluation without changing the program output.
  • The error messages are logged to standard output rather than standard error, making this unsuitable for shell scripts. To write to standard error, use eprintln!, ot to abort with a panic message, you can use panic!, expect! or unexpected!, depending on what kind of expression the type system needs.
  • The code to print the error message and exit is repeated unnecessarily.
  • Most importantly, we lose information about our errors! If there’s any kind of failure anywhere in the program, on any input, all we know is that the operation failed. Somewhere. We could easily have lost the information on the stack trace, too. If you ever have to debug a None that shouldn’t be there, you’ll curse yourself for not failing fast.

So, let’s see how this looks if we change to a Result type, whose Err value is a &'static str. The OPSymbol::eval function becomes:

    fn eval(i1: f64, i2: f64, op: &OPSymbol) -> Result<f64, &'static str> {
        match op {
            OPSymbol::ADD => Ok(i1 + i2),
            OPSymbol::SUB => Ok(i1 - i2),
            OPSymbol::MUL => Ok(i1 * i2),
            OPSymbol::DIV => {
                if i2 == 0.0 {
                    Err("Can't divide by 0!")
                } else {
                    Ok(i1 / i2)
                }
            }
            OPSymbol::EXP => Ok(i1.powf(i2)),
            _ => Err("Can't evaluate invalid symbol"),
        }
    }

This is a great use case to return an Err string, because a syntax error in a line of input is recoverable. The program prints the error message and then gives the user a chance to retype the line in the Read-Evaluate-Print-Loop.

This requires a cascade of changes to the program, but it enables the program to see a syntax error, print a message about what the error was, and read a new value instead of aborting.

On an Unrecoverable Error, Fail Fast

And be sure to do it from someplace where the stack trace will tell you enough about what went wrong. The worst thing you can do is throw away all the information you need to debug the problem and crash somewhere far away later.

Generally, None values should be for valid results that the caller handles. If you ever find yourself writing a return from an Err(_) or .is_err() block, or passing around an Err type of (), that’s a major code smell. If you’re not even looking at any of the information you had about the error, what was the point of using a Result type?

Depending on the context, you can make an error fail fast with a panic! or expect message. Sometimes, when the compiler makes you write exhaustive branches with the same type, you want to declare some unreachable!.

Avoid process::exit

You have many calls to this, and none of them are necessary.

If you need to print a message to standard error and abort the program, use panic!, or a more sugary synonym like unreachable! or .expect. However, nearly all of these should return a Result of Err, not halt the program.

If you want to return successfully from main, just return, or reach its closing brace.

Prefer &str to String Function Parameters

Sometimes, you really do want to consume a String object and re-use its memory. Maybe you call .into_bytes() on it, for example.

But if all you need is a view of something stringy, you should take a &str. This works on slices (like the one you get from .trim() in main), leaves the object usable and often saves you the overhead of cloning.

Here, you leave a comment saying that you are taking ownership because your main function of this one program doesn’t happen to need the input line after the call. But that’s not a good way to design an API. It results in an API that needs to change in many other programs (like one that trims whitespace from all lines, not just the commands q or quit).

Use More-Informative Names

Quick, without looking: what’s the difference between to_result and get_result? Does either produce a Result?

Run rustfmt

I don’t like all of its conventions, but it’s pretty standard for Rust, and would catch a few typos.

I also recommend against /// for comments.

Refactor get_result

The first change I’d make is to change the return type to a Result, so that all the branches that check for errors can become an if/else if block whose branches each return Err or Ok.

I’d also make the loop into a loop and check for the terminating conditions, which might be errors or a single Number value. What would really enhance the maintainability a lot, though, is if you pattern-match on slices of your Vec. Here’s one possible start:

// Convert vector of Tokens to a final result as a f64 number
fn get_result(mut tokens: Vec<Token<f64>>) -> Result<f64, &'static str> {
    use Token::{Number, Operator};

    loop {
        let n = tokens.len();

        if n == 0 {
            return Err(""); // Print a blank line in response.
        } else if let [Number(a)] = tokens[..] {
            return Ok(a);
        } else if n < 3 {
            return Err("Syntax error.");
        } else if let [Operator(op), Number(a), Number(b)] = tokens[n - 3..n] {
            match OPSymbol::eval(a, b, op.op_symbol) {
                Ok(c) => {
                    tokens[n-3] = Number(c);
                    tokens.truncate(n-2);
                }
                err => {
                    return err;
                }
            }
        } else {
            return Err("Logic error: Parsed to invalid tree!");
        }
    } // end loop
}

Note that, for this pattern-match to compile, you need to derive both Clone and Copy for Token and all of its components. Otherwise, you would need to borrow a slice of the Vec, preventing you from borrowing it mutably to resize it. From the comments, you had the same problem, and solved it another way. In this case, though, the elements are so small that copying is a zero-cost abstraction.

I also changed the definition of OpSymbol::eval to return the same Result type, and remove the borrow.

Also note that this approach does not work with your current implementation of the shunting-yard algorithm, since yours evaluates from left to right.

Parse into Reverse Polish Notation

That is, push the operator first, then the operands. This allows you to evaluate the stack from the top down, and push intermediate results back onto the same stack. This is much more efficient than removing and inserting elements in the middle of a Vec, which shifts the array multiple times.

If you really, truly need to burn a vector from both ends, use a VecDequeue.

There are various other ways to optimize the parser, but you don’t want to write parsers by hand as anything but a learning exercise anyway.

Store Only the Necessary Information in Each Instance

Currently, you store not only the identifier of each operator, but a copy of its precedence and left-or-right association. But these are always the same for each operation! There’s no such thing as a multiply token that has lower precedence than an addition. You should store only the type of operation on the stack.

Make Main a Read-Evaluate-Print Loop that Reports Errors

If you change the type of get_result to

fn get_result(mut tokens: Vec<Token<f64>>) -> Result<f64, &'static str>

(or Result<f64, String> if you want to be able to return a dynamic error message) and make all the cascading changes that requires, you can rewrite main along the lines of,

use std::io;

pub fn main() {
    for input in io::stdin().lines().map(Result::unwrap) {
        let trimmed_input = input.trim();

        if trimmed_input == "quit" || trimmed_input == "q" {
            return;
        }

        match to_result(trimmed_input) {
            Ok(result) => {
                println!("{:?}", result);
            }
            Err(msg) => {
                println!("{}", msg);
            }
        }
    } // end for
}

This accepts the input

18/0
18/9

and produces the output

Can't divide by 0!
2.0
\$\endgroup\$
6
  • \$\begingroup\$ Thank you for the detailed feedback. I disagree with 2 points: the excessive use of process::exit, i only call it once from main if user types "q" or "quit" to exit the process cleanly instead of looping indefinitely. The Vec<Token> is parsed into reverse polish notation already The advantage of reverse Polish notation is that it removes the need for order of operations and parentheses that are required by infix notation and can be evaluated linearly, left-to-right. Other than this I'll work on refactoring the code using your advice. \$\endgroup\$
    – ruinedme
    May 23, 2023 at 21:47
  • \$\begingroup\$ @ruinedme Okay, you’re not using process::exit much, although I’d still recommend that you just return from main instead. You got me. \$\endgroup\$
    – Davislor
    May 23, 2023 at 22:00
  • \$\begingroup\$ @ruinedme As for RPN: my recommendation is that you evaluate from right-to-left instead of left-to-right, as that lets you use Vec::push and Vec::shrink_to where you’re currently calling Vec::remove and Vec::insert four times per iteration, shifting the entire array each time. Call that whatever you like; reverse Hebrew? \$\endgroup\$
    – Davislor
    May 23, 2023 at 22:03
  • 1
    \$\begingroup\$ @Davislor I think you mean Vec::truncate not Vec::shrink_to as the latter will not affect the length of the Vec \$\endgroup\$
    – cafce25
    May 29, 2023 at 16:14
  • \$\begingroup\$ @cafce25 Whoops. Thanks! \$\endgroup\$
    – Davislor
    May 29, 2023 at 16:15

Your Answer

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

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