10
\$\begingroup\$

I am trying to use the practices that I have learnt from the rust book. Any advice is welcome. Please feel free to be as nitpicky as possible.

use std::fmt;
use std::io::{self, Write};

struct Item {
    name: String,
    done: bool,
}
impl fmt::Display for Item {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "{} [{}]", self.name, if self.done { 'X' } else { ' ' })
    }
}

fn main() {
    let mut todos: Vec<Item> = Vec::new();

    loop {
        let mut input = String::new();
        print!("> ");
        io::stdout().flush().unwrap();
        io::stdin()
            .read_line(&mut input)
            .expect("Failed to read line");
        let tokens: Vec<&str> = input.trim().split_whitespace().collect();

        match tokens[..] {
            ["add", item_name] => {
                let item_name = String::from(item_name);
                println!("Added item: {item_name}");
                let item = Item {
                    name: item_name,
                    done: false,
                };
                todos.push(item);
            }

            ["remove", item_name] => {
                let item_name = String::from(item_name);
                if let Some(index) = todos.iter().position(|x| (*x).name == item_name) {
                    todos.remove(index);
                }
                println!("Removed item: {item_name}");
            }

            ["toggle", item_name] => {
                let item_name = String::from(item_name);
                if let Some(index) = todos.iter().position(|x| (*x).name == item_name) {
                    todos[index].done = !todos[index].done
                }
            }

            ["show"] => {
                for item in todos.iter() {
                    println!("   {item}");
                }
            }

            ["exit"] => {
                println!("Exited applciation");
                break;
            }
            _ => println!("invalid input"),
        }
    }
}

\$\endgroup\$

2 Answers 2

9
\$\begingroup\$

Use cargo clippy to find common mistakes:

Redundant trim

In input.trim().split_whitespace() you can remove the trim(), since split_whitespace already ignores leading and trailing whitespace.

Unnecessary deref

In this closure: |x| (*x).name == item_name), you can remove the * since the compiler will dereference it automatically.

Explicit iter()

It's more idiomatic to write &todos instead of todos.iter().

Unnnecessary String creation

This line is repeated throughout your match arms: let item_name = String::from(item_name);. However, in most arms, you can elide it, since you can compare directly against the &str item_name.

Simpler Vec initialization

In the line let mut todos: Vec<Item> = Vec::new();, you can remove the explicit type and call the vec! macro for more concise code: let mut todos = vec![];

Typo

applciation should be application.

Inaccurate output

When removing an element, you always print a sucess message. It would make more sense to only print it inside the if so it only is shown after really removing an element. Additionally, a sucessful toggle should also get a sucess message for consistency.

eprintln! for errors

Errors should be printed to stderr via eprintln! and not via println!.

Final code

use std::fmt;
use std::io::{self, Write};

struct Item {
    name: String,
    done: bool,
}
impl fmt::Display for Item {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "{} [{}]", self.name, if self.done { 'X' } else { ' ' })
    }
}

fn main() {
    let mut todos = vec![];

    loop {
        let mut input = String::new();
        print!("> ");
        io::stdout().flush().unwrap();
        io::stdin()
            .read_line(&mut input)
            .expect("Failed to read line");
        let tokens: Vec<&str> = input.split_whitespace().collect();

        match tokens[..] {
            ["add", item_name] => {
                let item = Item {
                    name: String::from(item_name),
                    done: false,
                };
                todos.push(item);
                println!("Added item: {item_name}");
            }

            ["remove", item_name] => {
                if let Some(index) = todos.iter().position(|x| x.name == item_name) {
                    todos.remove(index);
                    println!("Removed item: {item_name}");
                }
            }

            ["toggle", item_name] => {
                if let Some(index) = todos.iter().position(|x| x.name == item_name) {
                    todos[index].done = !todos[index].done;
                    println!("Toggled item: {item_name}");
                }
            }

            ["show"] => {
                for item in &todos {
                    println!("   {item}");
                }
            }

            ["exit"] => {
                println!("Exited application");
                break;
            }
            _ => eprintln!("invalid input"),
        }
    }
}

Further ideas

  • Consider adding a help command for easier usage.
  • Consider adding support for saving the todos to and loading them from a file to preserve state over multiple runs.
\$\endgroup\$
2
  • \$\begingroup\$ Hello, I was trying to implement saving as you said. First I thought I could save it as a csv but people pointed out to me that it has too many edge cases. I was told to use something like sqlite. But that seems like overkill for a simple data like this. Do you have any recommendations? Especially something I can do without external modules. \$\endgroup\$
    – Doruk
    Mar 12 at 16:43
  • 1
    \$\begingroup\$ If you really want to avoid external libraries, you will probably have to use your own custom format for storing this and convert the todos to this format manually. But the simplest approach may be to use json (via the serde and serde_json libraries), since it's very convenient to serialize and deserialize Rust objects into json with them. \$\endgroup\$
    – Dornteufel
    Mar 12 at 18:54
7
\$\begingroup\$

This looks good! There’s no One True Coding Style, but since you seemed to be interested in fluent style, with iterators, I’ll go into how you could refactor that way.

Let’s factor out the part of the code that parses and executes the commands into a helper function. Let’s call it todo_cmd for now. The main() function can now become:

fn main() {
    io::stdin()
        .lines()                                 // Iterator over io::Result<String>
        .map(io::Result::unwrap)                 // Iterator over String
        .try_fold(Vec::<Item>::new(), todo_cmd); // Accumulates to an Option<Vec<Item>>
}

Much shorter, and the pattern we’re using is very explicit.

The Iterator::try_fold method is a little obscure, but what it does is take a state object, here initialized to an empty Vec::item, and pass it to a function, together with the next item in the sequence. In functional programming, a fold would return the new state, to be passed to the function next time. The classic example is to calculate a sum with

.fold(0, |accumulator, term| accumulator + term)

or a product with

.fold(1, |accumulator, term| accumulator * term)

But this is try_fold. It’s different because it passes the state (the current vector of items) to a function that possibly returns an updated state, but might also fail. If the function ever fails to return a state, the iteration stops. So the function signature is something similar to:

fn todo_cmd(mut todos: Vec<Item>, line: String) -> Option<Vec<Item>>

The motivation here is that there’s a command that stops the program, exit. If we get this command, we should return None. Otherwise, we should update todos and return the updated value. (Since todos is owned, this will move, and not copy, it.)

An implementation that works, and stays close to what you wrote (but which you could definitely improve on—it’s not very efficient):

fn todo_cmd(mut todos: Vec<Item>, line: String) -> Option<Vec<Item>> {
    print!("> ");
    io::stdout().flush().unwrap();

    if let Some(("", untrimmed)) = line.split_once("add ") {
        let trimmed = untrimmed.trim();
        todos.push(Item {
            name: trimmed.to_string(),
            done: false,
        });
        println!("Added item: {}", trimmed);
        Some(todos)
    } else if let Some(("", untrimmed)) = line.split_once("remove") {
        let trimmed = untrimmed.trim();
        if let Some(i) = todos.iter().position(|s| s.name == trimmed) {
            todos.remove(i);
            println!("Removed: {}", trimmed);
        } else {
            eprintln!("Could not find: {}", trimmed)
        }
        Some(todos)
    } else if let Some(("", untrimmed)) = line.split_once("toggle") {
        let trimmed = untrimmed.trim();
        if let Some(i) = todos.iter().position(|s| s.name == trimmed) {
            todos[i].done = !todos[i].done;
            println!("Set to {}: {}", todos[i].done, trimmed);
        } else {
            eprintln!("Could not find: {}", trimmed)
        }
        Some(todos)
    } else if "show" == line.trim() {
        todos.iter().for_each(|item| println!("{}", item));
        Some(todos)
    } else if "exit" == line.trim() {
        None
    } else {
        eprintln!("Unknown command: {}", line.trim());
        Some(todos)
    }
}

The main change to functionality is printing error messages to standard error. There are a small handful of clean-ups, such as removing a few let declarations.

There are several other ways to do it, for example, checking for exit with take_while and passing the resulting sequence to for_each.

Also keep this up top:

use std::fmt;
use std::io::{self, Write};

struct Item {
    name: String,
    done: bool,
}
impl fmt::Display for Item {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "{} [{}]", self.name, if self.done { 'X' } else { ' ' })
    }
}
\$\endgroup\$

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.