Rust: Command line menu in ASCII table

Question

Specific areas in which I'd love to get feedback:

• Is it good to have the Command struct own the Strings (description field)? Or since these strings aren't really meant to change, should I rather have them as &'static strs?
  • I initially used String but then switched to &'static str, and then I saw this SO post and switched back to String.
• Within the print_table function, am I complicating the code by having the max_lengths (for two fields of the Command struct) as a tuple instead of using two separate scalar variables?
• Also, is there a more idiomatic way of finding the maximum of lengths of a String field in array of structs?
• I've seen imports being placed at the top (e.g., use std::cmp::max) before the functions are used (max(1, 2)). I know that this keeps the code terse. However, I'm also a fan of qualified naming (e.g., in Python, it's encouraged to do import math; math.pow(x, y) instead of from math import pow; pow(x, y)). This way, to readers, there's no confusion about the source of a function. What's the general consensus of Rust community? Up to each dev?

fn main() {
 let commands: [Command; 3] = [
 Command {code: '1', description: String::from("Start your instance")},
 Command {code: '2', description: String::from("Stop your instance")},
 Command {code: 'q', description: String::from("Quit")},
 ];
 
 print_table(&commands);
 
 for cmd in commands.iter() {
 match cmd.code {
 '1' => println!("Starting instance"),
 '2' => println!("Stopping instance"),
 'q' => println!("Quitting"),
 _ => println!("Invalid option"),
 }
 }
}
fn print_table(commands: &[Command; 3]) {
 const BUFFER: usize = 2;
 const HEADING: (&str, &str) = ("Code", "Description");
 const VERTICAL_BAR: &str = "│";
 const HORIZONTAL_SEP: &str = "─";
 
 std::process::Command::new("clear").status().unwrap();
 
 let mut max_lengths: (usize, usize) = (0, 0);
 for cmd in commands.iter() {
 max_lengths.0 = std::cmp::max(max_lengths.0, cmd.code.len_utf8());
 max_lengths.1 = std::cmp::max(max_lengths.1, cmd.description.len());
 }
 max_lengths.0 = std::cmp::max(max_lengths.0, HEADING.0.len());
 max_lengths.1 = std::cmp::max(max_lengths.1, HEADING.1.len());
 
 let horizontal_lines: (&str, &str) = (
 &HORIZONTAL_SEP.repeat(max_lengths.0 + BUFFER),
 &HORIZONTAL_SEP.repeat(max_lengths.1 + BUFFER),
 );
 
 println!("╭{}┬{}╮", horizontal_lines.0, horizontal_lines.1);
 println!("{VERTICAL_BAR}{:^w1$}{VERTICAL_BAR} {:^w2$}{VERTICAL_BAR}", HEADING.0, HEADING.1, w1=max_lengths.0+BUFFER, w2=max_lengths.1+BUFFER-1);
 println!("├{}┼{}┤", horizontal_lines.0, horizontal_lines.1);
 for cmd in commands.iter() {
 println!("{VERTICAL_BAR}{:^w1$}{VERTICAL_BAR} {:<w2$}{VERTICAL_BAR}", cmd.code, cmd.description, w1=max_lengths.0+BUFFER, w2=max_lengths.1+BUFFER-1);
 }
 println!("╰{}┴{}╯", horizontal_lines.0, horizontal_lines.1);
}
struct Command {
 code: char,
 description: String,
}

Answer 1

I tend to favor &'static str when I can just for readability and for the same reason I declare things immutable when I can. The 'static lifetime is effectively the longest possible lifetime and for your code, so it doesn't invite the same lifetime issues for shorter-lived equivalents. Since you can freely replace one or the other and these strings are constants, I don't see a major advantage either way. The big thing with String is mutability, which you've said you don't need.

I think you're duplicating some code in your print_table as well as juggling parallel structures. I also usually reserve tuples for cases where I know the length of the concept I'm representing is always fixed. For this I think a vector might be more appropriate (or at least something that can be extended and iterated over quickly). I would consolidate the information of a heading together.

struct Heading {
 name: &'static str,
 length_fn: fn(&Command) -> usize
}

This will also make things more extensible if you ever add another column. I like to make extensive use of map to pipeline these types of calculations. I also chained some iterators here so you're not repeating max in the code.

I would usually break these down into their own functions and move the Heading vec! declarations to elsewhere to clean it up, but I left that aside for conciseness. Examples follow

 let headings: Vec<Heading> = vec![
 Heading {
 name: "Code",
 length_fn: |cmd| cmd.code.len_utf8(),
 },
 Heading {
 name: "Description",
 length_fn: |cmd| cmd.description.len(),
 },
 ];
 let buffer_lengths: Vec<usize> = headings.iter()
 .map(|heading| commands.iter()
 .map(|command| { (heading.length_fn)(&command) }).into_iter()
 .chain(std::iter::once(heading.name.len()))
 .max().unwrap_or(0) + BUFFER // Do your own error case
 ).collect();
 let horizontal_lines: Vec<String> = buffer_lengths.iter()
 .map(|&len| HORIZONTAL_SEP.repeat(len)).collect();

then reference stuff like

 println!("╭{}┬{}╮", horizontal_lines[0], horizontal_lines[1]);
 println!("{VERTICAL_BAR}{:^w1$}{VERTICAL_BAR} {:^w2$}{VERTICAL_BAR}", headings[0].name, headings[1].name, w1 = buffer_lengths[0], w2 = buffer_lengths[1] - 1);

I recognize the horizontal_lines is also a parallel structure to headings and you can probably do more consolidation refactoring how the text is printed. For two headings I might leave it for now.

Thinking more on it, I'd also probably end up adding moving these into Heading itself and a new that takes the name, length closure, and the commands which does this calculation. Then Heading can be responsible for that data. That would do away with the obviously nested map. The important thing, though, is to move into iterators to streamline things.

For imports, I think it's up to you on how and I've seen both ways. Clarity and readability are the important things. If it's a one-off I might leave the full one in for clarity unless I feel it's very obvious, but if it's repeated I usually do the import.

I feel like in Python this is more important because of how it was cobbled together. What gets exported in Python libraries can become pretty big due to how namespacing (or, really, lack thereof) is done. You will find counterexamples of this in both languages, of course.

I personally tend to write many shorter pieces of code rather than fewer and longer, so name collisions on imports aren't something I run across frequently. Salt and pepper to taste.

Answer 2

Separate main program and library

Currently you have everything in one unit file. This is fine for small applications that don't do much, but it will decrease re-usability of your code.

Make your code generic

Currently the print_table() function is limited to your custom Command structs, which is unnecessary. You could refactor it to make it usable for any two-tuple slices whose elements implement Display.

Make constants global

Declaring constants within a function looks weird to me. While the language allows it, I'd expect the function's business logic there. In your current case, the constants are only being used by said functions, so one could argue that it's okay to declare them there. But for future refactoring, I'd prefer to declare them at the top of the respective unit file.

Derive appropriate traits

Command should at least implement Debug to, well, allow debugging if and when the project grows.

Implement an API

Command should at least implement new() and maybe some getters for its fields. This also makes it possible to outsource it into a library unit.

Suggested:

Cargo.toml

[package]
name = "table"
version = "0.1.0"
edition = "2021"
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
[dependencies]

lib.rs

use std::fmt::Display;
const BUFFER: usize = 2;
const VERTICAL_BAR: &str = "│";
const HORIZONTAL_SEP: &str = "─";
#[derive(Debug)]
pub struct Command<'desc> {
 code: char,
 description: &'desc str,
}
impl<'desc> Command<'desc> {
 #[must_use]
 pub const fn new(code: char, description: &'desc str) -> Self {
 Self { code, description }
 }
 #[must_use]
 pub const fn header() -> (&'static str, &'static str) {
 ("Code", "Description")
 }
 #[must_use]
 pub const fn code(&self) -> char {
 self.code
 }
 #[must_use]
 pub const fn description(&self) -> &'desc str {
 self.description
 }
}
pub fn print_table<L, R>(rows: &[(L, R)], header: Option<(&str, &str)>)
where
 L: Display,
 R: Display,
{
 let mut max_lengths: (usize, usize) = (0, 0);
 for (lhs, rhs) in rows {
 let lhs = lhs.to_string();
 let rhs = rhs.to_string();
 max_lengths.0 = std::cmp::max(max_lengths.0, lhs.len());
 max_lengths.1 = std::cmp::max(max_lengths.1, rhs.len());
 }
 if let Some((lhs, rhs)) = header {
 max_lengths.0 = std::cmp::max(max_lengths.0, lhs.len());
 max_lengths.1 = std::cmp::max(max_lengths.1, rhs.len());
 }
 let horizontal_lines: (&str, &str) = (
 &HORIZONTAL_SEP.repeat(max_lengths.0 + BUFFER),
 &HORIZONTAL_SEP.repeat(max_lengths.1 + BUFFER),
 );
 println!("╭{}┬{}╮", horizontal_lines.0, horizontal_lines.1);
 if let Some((lhs, rhs)) = header {
 println!(
 "{VERTICAL_BAR}{lhs:^w1$}{VERTICAL_BAR} {rhs:^w2$}{VERTICAL_BAR}",
 w1 = max_lengths.0 + BUFFER,
 w2 = max_lengths.1 + BUFFER - 1
 );
 println!("├{}┼{}┤", horizontal_lines.0, horizontal_lines.1);
 }
 for (lhs, rhs) in rows {
 println!(
 "{VERTICAL_BAR}{lhs:^w1$}{VERTICAL_BAR} {rhs:<w2$}{VERTICAL_BAR}",
 w1 = max_lengths.0 + BUFFER,
 w2 = max_lengths.1 + BUFFER - 1
 );
 }
 println!("╰{}┴{}╯", horizontal_lines.0, horizontal_lines.1);
}

main.rs

use table::{print_table, Command};
const COMMANDS: [Command; 3] = [
 Command::new('1', "Start your instance"),
 Command::new('2', "Stop your instance"),
 Command::new('q', "Quit"),
];
fn main() {
 print_table(
 &COMMANDS.map(|command| (command.code(), command.description())),
 Some(Command::header()),
 );
 for cmd in &COMMANDS {
 match cmd.code() {
 '1' => println!("Starting instance"),
 '2' => println!("Stopping instance"),
 'q' => println!("Quitting"),
 _ => println!("Invalid option"),
 }
 }
}

• m01010011: Very helpful. Thanks. As for declaring constants at the top of the file, yeah, the general rule I follow within my code is that if something is used in only one function, it's better to declare it there. Also, in Python, fetching variable from local scope is (slightly) faster than getting it from global / module level. Since you didn't make any mention of such behaviour for Rust, is it right to assume that it doesn't matter? Is it because, like in C, the constants get replaced during compilation?
• Richard Neumann: I build my above code for --release on stable-x86_64-unknown-linux-gnu and with the constants moved back into the function. It compiled to the exact same binary.

Answer 3

The String Table

In this program, all your strings are constants, so you could store them as a const array:

struct Command {
 code: char,
 description: &'static str,
}
const COMMANDS: [Command; 3] = [
 Command {
 code: '1',
 description: "Start your instance",
 },
 Command {
 code: '2',
 description: "Stop your instance",
 },
 Command {
 code: '3',
 description: "Quit",
 },
];

However, in many real-world situations, you will want to look up your localized strings in a database of translations that you load at runtime. This is a good use case for lazy static initialization. You can initialize the lookup table, at runtime, in a thread-safe way, the first time it’s used, and the localized strings have the 'static lifetime.

This approach allows your Command objects to store a &'static str, and still use localized strings loaded from a translation file at runtime.

Here, I use the once_cell crate to implement this, but lazy_static would also work, and as of 1.77.0, there’s an experimental std::sync::LazyLock API.

use std::collections::HashMap;
struct Command {
 code: char,
 description: &'static str,
}
fn translation_build() -> HashMap<&'static str, String> {
 let mut map = HashMap::new();
 for (key, value) in [
 ("START", "Start your instance"),
 ("STOP", "Stop your instance"),
 ("QUIT", "Quit"),
 ("STARTING", "Starting instance"),
 ("STOPPING", "Stopping instance"),
 ("QUITTING", "Quitting"),
 ("UNKNOWN_CMD", "Invalid option"),
 ] {
 map.insert(key, value.to_string());
 }
 map.shrink_to_fit();
 map
}
fn translation_lookup(key: &str) -> Option<&'static str> {
 use once_cell::sync::Lazy;
 static STRING_TABLE: Lazy<HashMap<&'static str, String>> = Lazy::new(translation_build);
 (&*STRING_TABLE).get(key).map(String::as_str)
}
fn main() {
 let commands = [
 Command {
 code: '1',
 description: translation_lookup("START").unwrap_or("START"),
 },
 Command {
 code: '2',
 description: translation_lookup("STOP").unwrap_or("STOP"),
 },
 Command {
 code: '3',
 description: translation_lookup("QUIT").unwrap_or("QUIT"),
 },
 ];
 for c in commands {
 println!(" {}: {}", c.code, c.description);
 }
}

If you want to make COMMANDS a static global variable, you can initialize it as lazy static, outside of main.

Finding the Greatest Length

The approach you use works, but you could also do it in fluent style with iterators.

In functional programming, this operation is a map-reduction using the max function.

use core::cmp::max;
fn main() {
 let commands = [
 Command {
 code: '1',
 description: translation_lookup("START").unwrap_or("START"),
 },
 Command {
 code: '2',
 description: translation_lookup("STOP").unwrap_or("STOP"),
 },
 Command {
 code: '3',
 description: translation_lookup("QUIT").unwrap_or("QUIT"),
 },
 ];
 let command_width = commands
 .iter()
 .map(|c| c.description.len())
 .reduce(max)
 .unwrap();
 println!("╭──┬{:─<command_width$}╮", "");
 for c in commands {
 println!("│{:>2}│{:·^command_width$}│", c.code, c.description);
 }
 println!("╰──┴{:─<command_width$}╯", "");
}

This prints the following:

╭──┬───────────────────╮
│ 1│Start your instance│
│ 2│Stop your instance·│
│ 3│·······Quit········│
╰──┴───────────────────╯

Or you might prefer to express this more concisely, as an accumulating fold:

 let command_width = commands
 .iter()
 .fold(0, |x, c| max(x, c.description.len()));

This is only slightly more complicated than the classic sum:

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

or product:

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

You could also map-reduce a pair of values in a single pass, using a closure.

 let (_key_bytes, command_width) = commands
 .iter()
 .map(|c| (c.code.len_utf8(), c.description.len()))
 .reduce(move |(a, b), (c, d)| (max(a, c), max(b, d)))
 .unwrap();

• glampert: We could make translation_lookup always return a valid string so we don't have to repeat the lookup key twice in the unwrap_or. If it fails it could just return the lookup key instead. Or if we want to have the ability to let the caller handle the error then the API could be split into a translation_lookup and a try_translation_lookup for example.
• Davislor: @glampert Those are all good ideas, if you want to turn this into production code!