Review of a Todo program using CLI and formatting

Here's my Todo List program

use clap::{Parser, Subcommand};
use colored::Colorize;
use rusqlite::{params, Connection};
use rusqlite::types::Null;
use time::{PrimitiveDateTime, OffsetDateTime, Duration, Date};
use time::macros::format_description;

fn slice_to_string(slice: &[i32]) -> String {
    let mut str = String::new();
    for (i, item) in slice.iter().enumerate() {
        str.push_str(item.to_string().as_str());
        if i <= slice.len() - 2 {
            str.push_str(", ");
        }
    }
    str
}

#[derive(Parser)]
#[command(version, about, long_about = None)]
pub struct Cli {
    #[command(subcommand)]
    command: Option<Commands>,
}

#[derive(Subcommand)]
enum Commands {
    Add { item: String },
    Complete { id: i32 },
    Remove { id: i32 },
}

fn add_item(item: String, conn: &Connection) -> Result<(), Box<dyn std::error::Error>> {
    println!("Do you want to add a due date? (y/n)");
    let str = read_line()?;
    if str.chars().nth(0) == Some('y'){
        println!("What date do you want to add a due date (YYYY-MM-DD)");
        let format = format_description!("[year]-[month]-[day]");
        let date_time = Date::parse(&read_line(), format)?.midnight();
        conn.execute("INSERT INTO items (ITEM, DUE_DATE, IS_COMPLETED) VALUES (?, ?, ?)", params![item, date_time, false])?;
    } else {
        conn.execute("INSERT INTO items (ITEM, DUE_DATE, IS_COMPLETED) VALUES (?, ?, ?)", params![item, Null, false])?;
    }
    Ok(())
}

fn read_line() -> Result<String, Box<dyn std::error::Error>> {
    let mut buf = String::new();
    std::io::stdin().read_line(&mut buf)?;
    Ok(buf)
}

fn list_items(conn: &Connection) -> Result<(), Box<dyn std::error::Error>> {
    let mut stmt = conn.prepare("SELECT * FROM items")?;
    let rows = stmt.query_map((), |rows| Ok((rows.get::<_, i32>(0)?, rows.get::<_, String>(1)?, rows.get::<_, PrimitiveDateTime>(2)?, rows.get::<_, bool>(3)?)))?;
    println!("{0: <10} | {1: <50} | {2: <20} | {3: <10}", "ID", "Description", "Due Date", "Status");
    let row_vec = rows.map(|rows| rows.unwrap()).collect::<Vec<_>>();
    let now = PrimitiveDateTime::new(OffsetDateTime::now_local()?.date(), OffsetDateTime::now_local()?.time());
    for row in row_vec.iter() {
        let row = row;
        let id = row.0;
        let description = &row.1;
        let due_date = row.2;
        let is_completed = row.3;
        if !is_completed {
            if now < due_date && due_date < now + Duration::days(3) {
                println!("{0: <10} | {1: <50} | {2: <20} | {3: <10}", id.to_string().yellow(), description.yellow(), due_date.to_string().yellow(), is_completed.to_string().yellow());
            } else if now > due_date {
                println!("{0: <10} | {1: <50} | {2: <20} | {3: <10}", id.to_string().red(), description.red(), due_date.to_string().red(), is_completed.to_string().red());
            } else {
                println!("{0: <10} | {1: <50} | {2: <20} | {3: <10}", id, description, due_date, is_completed);
            }
        }
    };
    let overdue_items = &row_vec.iter().filter(|row| row.2 < now).map(|row| row.0).collect::<Vec<_>>()[..];
    let close_items = &row_vec.iter().filter(|row| now + Duration::days(3) > row.2 && row.2 > now).map(|row| row.0).collect::<Vec<_>>()[..];
    if overdue_items.len() > 0 { println!("{} {:?} {}", "Items".red(), slice_to_string(overdue_items).red(), "are overdue! Finish them quickly!".red()); }
    if close_items.len() > 0 { println!("{} {:?} {}", "Items".yellow(), slice_to_string(close_items).yellow(), "are close to deadline! Finish them quickly!".yellow()); }
    Ok(())
}

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let cli = Cli::parse();
    let conn = Connection::open("./todo.db")?;
    conn.execute("CREATE TABLE IF NOT EXISTS items (\
         ID INTEGER PRIMARY KEY AUTOINCREMENT UNIQUE,\
         ITEM TEXT NOT NULL UNIQUE,\
         DUE_DATE TEXT,\
         IS_COMPLETED INTEGER NOT NULL\
    )", ())?;
    match cli.command {
        Some(Commands::Add { item, }) => {
            add_item(item, &conn)?;
        }
        Some(Commands::Complete { id }) => {
            conn.execute("UPDATE items SET IS_COMPLETED = 1 WHERE ID = ?", params!(id))?;
        },
        Some(Commands::Remove { id }) => {
            conn.execute("DELETE FROM items WHERE ID = ?", params!(id))?;
        }
        None => {
            list_items(&conn)?;
        }
    }
    Ok(())
}
1 Like

Not bad at all - for a first/test project ever. Review-wise, I'll separate it into three parts.

Minor details

  1. use !overdue_items.is_empty() instead of overdue_items.len() > 0: it's easier to read
  2. avoid polluting your code with unnecessary references: calling .iter() on row_vec will implicitly borrow it immutably, making the &row_vec beforehand completely redundant [1]
  3. use pattern matching: if let Some('y') = str.chars().nth(0) {}, instead of regular comparison by equality: str.chars().nth(0) == Some('y'); when it's an option
  4. same goes for tuples: let (id, desc, due, is_done) = row; (don't let row = row;)
  5. keep the common types at hand: type Result = Result<(), Box<dyn std::error::Error>> + fn new_fn() -> Result<()> {} is much easier to both handle and change up later on
  6. don't shy away from raw r(#) string literals: they were made for multi-line strings
compare
// before

"CREATE TABLE IF NOT EXISTS items (\
     ID INTEGER PRIMARY KEY AUTOINCREMENT UNIQUE,\
     ITEM TEXT NOT NULL UNIQUE,\
     DUE_DATE TEXT,\
     IS_COMPLETED INTEGER NOT NULL\
)"

// after

r#"CREATE TABLE IF NOT EXISTS items (
     ID INTEGER PRIMARY KEY AUTOINCREMENT UNIQUE,
     ITEM TEXT NOT NULL UNIQUE,
     DUE_DATE TEXT,
     IS_COMPLETED INTEGER NOT NULL
)"#

Iterators

  1. keep your lines short, especially when using iterators with chaining: trying to make sense of the .iter() with .filter(|row| <f>) with .map(|row| <m>) with .collect::<Vec<_>() which is itself referenced as a slice with &<the whole chain above>[..] is just ... painful.
  2. don't mix inner unwrap()'s (as in rows.map(|r| r.unwrap())) with outer ? - if your fn() already supports the ? use the regular for r in rows { row_vec.push(r?); } instead.
  3. the turbofish ::<_, i32> and ::<Vec<_>> and ::<_, PrimitiveDateTime> might help keep your code slightly more concise, all the while making the job of whoever has to review / change / contribute to your code (including you) a whole lot more excruciating later on

There's nothing wrong with plain for x in abc { ... }. Throw in a Vec::new() + .push(next) and you'll have yourself an infinitely more readable version of whatever chain you had in mind. Should you insist on iterators, at the very least move every subsequent call to a new line:

let overdue_items: Vec<_> = row_vec.iter()
    .filter(|row| row.2 < now)
    .map(|row| row.0)
    .collect();
// any `Vec<T>` is implicitly coerced to a `[T]` on `&`
... .slice_to_string(&overdue_items).

Structure

This part has got to be your weakest spot yet, even assuming that you might have had to mix and throw everything together in one single file, just for the sake of the review. Specifically:

  1. you have no separation of concerns: low-level details (SQL) are mixed with the high-level intent (add_item); the utilities (read_line and slice_to_string) are placed alongside the Cli parser; everything is everywhere and nothing has any clear distinct purpose of its own
  2. the intent itself barely matches the implementation: the add_item is a check_for_date + get_input + Date::parse (maybe) + the conn.execute; why even call it add_item then?
  3. DRY might have its limitations, but to have the exact same println!("{0: <10} | {1: <50} | {2: <20} | {3: <10}", ...) in 4 different places which handles the exact same 4 pieces of data (id and the rest) is definitely not the best way to go about it; at least extract it:
// generic over all types, convertible to `String`
fn print_row<C1,C2,C3,C4>(
  col1: C1, col2: C2, 
  col3: C3, col4: C4, 
  is_warn: bool, 
  is_expired: bool
) -> String 
where C1: ToString, C2: ToString, C3: ToString, C4: ToString 
{
    let text = format!("{0: <10} | {1: <50} | {2: <20} | {3: <10}", 
        col1.to_string(), col2.to_string(), col3.to_string(), col4.to_string());
    if is_warn { return format!("{}", text.yellow()); }
    if is_expired { return format!("{}", text.red()); }
    text
}

An alternative might a custom macro_rules! that would inline the .yellow() and .red() calls instead, but we've already covered more than enough, for the time being.

To sum it up: not bad. Could use a bit of an improvement on the readability, consistency and structure aspect - but those will come with time and experience: as long as you apply yourself.


  1. after scrolling for god knows how long to the right I see that you were borrowing not the row_vec itself but the Vec you've just collect()-ed from the whole chain ... just, why? ↩︎

3 Likes

Is there benefit to the minor detail number 3? I think one should only use the let when it's defining a new variable?

  • comparison by == requires the type in question to implement std::ops::Eq, which is built in the std for i32 and Option<usize>, but not for any custom struct's or enum's; the latter can still be match'ed + destructured with let Enum::Struct { .. } = val though
  • matching on the let does (seem to) run quite a bit faster than the == in debug builds [1]
  • last but not least - the let $pat is (virtually) always easier on the eyes, readability-wise:
let Some('a'|'b'|'c') = c else { continue }; 
// compare to
if c == &Some('a')
|| c == &Some('b') 
|| c == &Some('c') { 
    a_count += 1; 
}

Nope, not necessarily. You can use a let to simply check if a given type matches any given pattern, compatible with it (see @tczajka's post). All of these are perfectly valid:

let a = 42; // a: i32
let ref mut b = 12; // b: &mut i32
let Ok(Some(..)) = res_opt else { return Err(...); } // diverging case
let from_opt = if let None = Some(true) { skip() } else { do_not_skip() };

  1. with both of them being +/- equal in release mode; but only as long as the let pattern is of <a>|<b>|<c> kind, and not a range-like <a>...<z>: which does seem to run even faster in debug mode, but loses a few speed points in an optimized release build (no idea why) ↩︎

Probably also worth mentioning that the if let syntax will be even more useful when you can && together multiple of them along with other boolean expressions using if-let-chains. I don't know when it will be released, but I suspect pretty soon since it looks done.

1 Like

I didn't quite get how you could improve Iterator point 3. Could you please elaborate?

Compare

version #1
let rows = stmt.query_map((), |rows| Ok((rows.get::<_, i32>(0)?, rows.get::<_, String>(1)?, rows.get::<_, PrimitiveDateTime>(2)?, rows.get::<_, bool>(3)?)))?;

to

version #2
let rows = stmt.query_map((), |row| {
    let id: i32 = row.get(0)?;
    let desc: String = row.get(1)?;
    let due_date: PrimitiveDateTime = row.get(2)?;
    let is_done: bool = row.get(3)?;
    Ok((id, desc, due_date, is_done))
})?;
2 Likes