Request for code review on my first project


#1

I’ve recently started using Rust and was hoping I could get some feedback on my code. I’m sure much of it isn’t idiomatic Rust and certain things could be cleaner. The whole project is just under 1000 lines of code, but I’d like to point out a few things that I’m unsure of.

I’ve written a BASIC interpreter (github repo here) in Rust.

A few of the pain points I’d like guidance on:

  • I’ve defaulted to using String in most places, I don’t really have an intuition when to use str instead.

  • In the parse_and_eval_expression function in src/evaluator.rs (see the source) I have a lot of copy and pasted code that was modified slightly for each case. The combination of handling the types in BASIC and Rust’s types made this ugly. In my Python version of this code, I just had something that returned a lambda for the operation and with Python’s “Duck Typing” it just handled it. I would have liked to have a trait that was implemented for each operator that handled this, but I wasn’t sure how to handle the different argument types.

  • In general, the right-ward march of my code. I like that Rust’s type system makes me deal with errors with Reesult and Option traits. But this tends to make my code be deeply nested and it makes me feel like there is something I’m not getting.

  • My use of clone() feels a little bit too much. I admit I tended to use it in places to please the borrow checker, but maybe there are more fundamental patterns that I’m missing out on so that it isn’t always the most appropriate thing to do.

I know I’m asking a lot, so I appreciate it if anyone can offer any amount of time to help me out. I’ve enjoyed my experience with Rust so far and I’d like to spend some more time with it.


#2

Hey Travis! Good to see you on this forum :slight_smile:

I don’t have a lot of useful commentary, but to speak to your second point, there is the new ? operator which helps a lot.

For example, where you have something like:

if i32::from_str(operand1.as_str()).is_ok() {
    ...
} else {
    return Err("Foo");
}

You can instead write something like:

let num = i32::from_str(operand1.as_str()).ok_or("Foo")?; // notice the ? at the end

and then you have num in scope as the i32. See the docs for Option::ok_or. I actually just learned about it yesterday and it’s made my code a lot nicer :slight_smile:

I also see a lot of duplication that can probably just be cleaned up with a few more let statements, like in the example that I took from your code above, you’re invoking from_str both in the if expression and in the then body – you can extract that out.

Hope this helps!


#3

Line 25 is a perfect use case for while let:

while let Some((pos, ch)) = char_iter.next() { ...

Given that your lexer doesn’t care about unicode, you could do your parsing in bytes instead of chars. One of the beautiful things about UTF-8 is that all ACII bytes have the same meaning in unicode. So, if you process your file byte-wise, you’ll never acidentially mistake a part of some unicode byte sequence as an ASCII character.


In lexers, I generally use:

  • String for tokens that I always modify/create from scratch.
  • Cow<str> for tokens that I sometimes modify but usually just copy from the input (useful when you need to handle escape sequences but that may fall under parsing).
  • &str for tokens that appear verbatim in the input string.

Finally, I lex lazily (by implementing a pull-based iterator). This makes it possible to write really fast lexers that allocate infrequently (or never). Take a look at the pulldown-cmark parser (not mine) for inspiration.


#4

Other fantastic Option/Result methods are map, and_then, and unwrap_or_else. And similar to while let, there is also an if let ... else .... To get yourself into the right mindset, try to play "unwrap golf"; you won’t necessarily be able to eliminate all unwraps, but try to eliminate as many as you can.

On another note, seeing code repeated at the beginning of a bunch of matches kinda bothers me. In particular, all the stack.len() checks and matches here:

Some(token::Token::Bang) => {
    if stack.len() >= 1 {
        match stack.pop().unwrap() {

                    \\ ...

Some(token::Token::GreaterThanEqual) => {
    if stack.len() >= 2 {
        match (stack.pop().unwrap(), stack.pop().unwrap()) {

Matching on something like an enum OperatorKind { Unary, Binary } first could help refactor and lift out these bits of duplicated code.