How would you write that code in a idiomatic way?

Hey! I am new here in the community and in Rust, and I was wondering how you would rewrite my code in an idiomatic way :confused: :

use std::io;

#[derive(Debug)]
pub enum Input {
    // numbers
    Number(u32),
    // Break when user type `/`
    Break,
    Error,
}

impl Input {
    /*
     */
    pub fn from(answer: String) -> Self {
        if answer.trim() == "/".to_string() {
            return Self::Break;
        }
        match answer.trim().parse::<u32>() {
            Ok(num) => Self::Number(num),
            Err(_) => Self::Error,
        }
    }
}

pub fn ask_numbers() -> Vec<u32> {
    // storing numbers
    let mut list = Vec::new();
    loop {
        println!("Enter u32 and enter `/` to end the list");

        let mut answer = String::new();

        io::stdin()
            .read_line(&mut answer)
            .expect("Failed to read input");

        let input = Input::from(answer);

        match input {
            Input::Number(num) => list.push(num),
            Input::Break => break,
            Input::Error => continue,
        };
    }
    list
}

What I am trying to do here is asking the user to enter a list of numbers to be stored in a vector.
If the user enters something else than a number, they will get an error. and asked to enter a number again.
In order for the loop to end, the user will have to enter / to break the loop.

It has only been 3 days that I am learning Rust so I don't have much of resources to help myself, so if you have any advices, links (youtube channels, etc... other than "the book") to learn Rust, I'll be glad to get them :wink:

Thanks in advance,

Noah

there are 2 things I'd change in the above code, first is about the Error variant you have in your enum.
the idiomatic way to pass errors is to use a Result type, such as Result<Input, ()>, where your input would be enum Input { Number(u32), Break }.

second, in the Input::from method, you're taking ownership of the string, but there's no reason to do that. rather, use a borrowed type, such as &str.
actually, instead of writing manually writing that method, you could implement the FromStr trait. then you could either Input::from_str(&str), or use the parse method from the string: String::parse::<Input>.

Also, you're allocating a new string every time the loop reruns, it would be better to alocate the string before entering the loop, then calling String::clear inside the loop.

Also, and this is optional, I think that the more "rusty" idiomatic way would for your ask_numbers function return an iterator, so: fn ask_numbers() -> impl Iterator<Item = Input>, that way you could use combinators on the result.

1 Like

Thanks mate that's really helpful!.
I haven't learned yet about trait nor iterator (I am at chapter 9 or 10 of the book) but that seems legit.
May I ask how you learnt Rust @jvcmarcenes ?

A smaller point: if all uses of answer in a function are answer.trim(), then I would put

let answer = answer.trim();

at the start of the function body rather than calling trim each time answer is used.

3 Likes
pub fn from(answer: &String) -> Result<Self, ()> {
        let answer = answer.trim();

        if answer == "/".to_string() {
            return Ok(Self::Break);
        }
        match answer.parse::<u32>() {
            Ok(num) => Ok(Self::Number(num)),
            Err(_) =>, Err(()),
        }
    }
match input {
            Ok(Input::Number(num)) => list.push(num),
            Ok(Input::Break) => break,
            _ => continue,
        };
}

@jvcmarcenes Is it something you'd write?

You shouldn't need "/".to_string(). In Rust this to_string() is for a specific kind of string: the owned one. But owned strings are needed only if you're going to keep them (e.g. store in a struct) or return from the function. They generally don't need to be owned to just compare with another string.

4 Likes

what 'd you do instead?

String::from("/") 

?

Under my interpretation, Break isn't an error, and this is what I put together. You don't need any owned Strings and can just used borrowed or literal &strs. You could move Break to the error enum if that's you're interpretation.

The traits implemented (From, FromStr) are things you learn about the standard library. As for that ParseIntError which I wrapped up, it's not something I knew about ahead of time. Instead I did something like this:

pub enum ParseInputError {
    ParseIntError, // No data yet
}
// No From implementation

impl FromStr for Input {
/* ... */
            Err(e) => Err(e), // No .into() yet

And the compiler told me the name of the type:

error[E0308]: mismatched types
  --> src/main.rs:28:27
   |
28 |             Err(e) => Err(e),
   |                       --- ^ expected enum `ParseInputError`, found struct `ParseIntError`

(Then I added the data part to the enum and the next compiler error told me where to import it from, then I implemented From and added .into().)

You could also implement Display and Error for the error enum.

There are crates like thiserror and anyhow to make error handling less boilerplate-y.

Or you could put off structured error handling until later, and just use the existing ParseIntError, since that's the only error type you have so far. Or maybe even Box<dyn Error>, which you can convert other error types into.

2 Likes

I haven't learned yet about trait nor iterator (I am at chapter 9 or 10 of the book) but that seems legit.

You'll get there, don't worry

May I ask how you learnt Rust @jvcmarcenes ?

I've been coding in rust for over a year now, and I'm definetely no expert.
But I learned with online resources, both the official rust book and the Easy Rust book for my introduction to the language, but also Rust by Example and the Rust Cookbook for common patterns and idiomatic rust, and The Rustonomicon for harder and low level concepts (it really helped me develop a better understanding of the ownership system and lifetimes, I really suggest it after you're done with the rust book). I also watched a few youtube channels, (Let's get Rusty, Crust of Rust by Jon Gjengset, Ryan Levick).
I also tried doing a lot of personal projects in rust, sometimes hitting some hard barriers and asking for help online or finding more specific resources to the problems I was trying to solve.

There are a few things I'd change here, I'll copy the code and comment it, and then post a playground link with how I'd write it.

// i would would use the FromStr trait, but since you haven't gotten there, this is okay
// also, I'd write that argument type as &str, it is more flexible than &String,
//    here's some explanation: https://www.ameyalokare.com/rust/2017/10/12/rust-str-vs-String.html
pub fn from(answer: &String) -> Result<Self, ()> {
  let answer = answer.trim();
  
  // I wouldn't use .to_string() for this, it allocates which is a big performance hit
  // it is better to do a match and compare the slices
  if answer == "/".to_string() {
    // I'm being pedantic, but unless the enum has a lot of generics, I don't like using the name Self for it
    // I feel it obscures the type and make it less readable
    return Ok(Self::Break);
  }
  
  // this is okay, but I would use .map and .map_err functions, I feel they're cleaner (but this is personal opnion)
  // I also would foward the error returned by .parse, there's no reason for us to hide that from the caller
  //   especially if we aren't handling it here.
  match answer.parse::<u32>() {
    Ok(num) => Ok(Self::Number(num)),
    Err(_) =>, Err(()),
  }
}

here's the playground link (I tried to put some comments there, but it wouldn't allow me, if there's any questions about the code, feel free to ask them)

1 Like

Out of curiosity, what do you mean here? Did you have some kind of error when commenting?

I wrote the code, generated a permalink to the playground, changed the code to put some comments, tried to regenerate the permalink but the playground just gave me the old link and I lost the comments...

Link is regenerated when you click Share button, it's not updated automatically.

1 Like

@quinedot that is such a complete and helpful answer which I learnt a lot from! Ta mate :wink:

@jvcmarcenes I'm starting think Rust has the best community ever :star_struck:

Thanks very much for the ressources Ima have a look at it rn!

Btw I'd like to mark both (@quinedot @jvcmarcenes ) of your answers as solution can I do it?

1 Like

My 2c: Rust Playground

I put all the things I changed in the comment at the top. I didn't read other people's suggestions to avoid being influenced by them, so there will almost certainly be overlap.

EDIT I've read other people's comments now. I do like @jvcmarcenes solution - FromStr is an abstraction that matches exactly what you're trying to do so it makes sense to use it.

Thank you for the acknowledgement! This forum only supports marking one, but getting marked as the solution or not isn't something I would mind or not.

2 Likes

Just "/". You don't need to allocate it on the heap and give it capacity to grow. You can compare other strings with a read-only literal just fine.