Feedback about catsay(a cowsay similar) code i've made

[dreisss]

Hi there, i just started learning rust and as a challenge i decided to create a cowsay-similar, and now I want to improve the code as much as possible.

I think I've done all I can, so now I want some other feedback. If any one could help me with this I would be so grateful.

here is the github repo: https://github.com/dreisss/rust-catsay
and the crate link: https://crates.io/crates/catsay-rs

Ps: Sorry for any english mistake..

[afetisov]

• Start with running cargo clippy on your project and fixing all warnings.

• Specify edition = "2021" in Cargo.toml. If you create a project using cargo init, proper default Cargo.toml will be created for you. Your IDE should also setup the initial project correctly (you use an IDE, right?).

• result_string.push_str(" ".repeat(length - string.len()).as_str());
  

  This could be result_string.push_str(&" ".repeat(length - string.len())). &String dereferences to &str as needed.

• src/modules/utilities.rs:fill could be done via format! macro. It has options to align your formatted value.

• pub fn into_iter(&self) -> IntoIter<String> {
    self.list.clone().into_iter()
  }
  

  You should implement the IntoIterator trait, rather than adding an ad-hoc method.

• pub fn nth(&self, index: usize) -> Option<String> {
    self.into_iter().nth(index)
  }
  

  This could easily be a fast O(1) access to the element rather than linear search. You can implement the Index trait. Or even better, just make the list field public. Your structure has no invariants to uphold, you could easily expose the contents.

• fn print_bubble_top(hyphen_fill: &str) {
    println!("          _-{hyphen_fill}-_");
  }
  
  fn print_bubble_middle_line(line_fill: &str) {
    println!("         |  {line_fill}  |");
  }
  
  fn print_bubble_bottom(hyphen_fill: &str) {
    println!("        / ¯-{hyphen_fill}-¯");
  }
  
  fn print_bubble_middle(strings: &StringList) {
    for line in strings.into_iter() {
      print_bubble_middle_line(line.as_str());
    }
  }
  

  You know what they say, "write everything twice". There is no point in introducing micro-functions with a single use site, you just make the code harder to read.

• pub fn match_call(&self, caller: &str) -> bool {
    for call in self.callers {
      if call == caller {
        return true;
      }
    }
    return false;
  }
  

  This could be self.callers.iter().any(|call| *call == caller).

• pub fn get(strings: &StringList) -> Option<Command> {
    let caller: String = Command::get_caller(strings).unwrap_or("".to_string());
    for command in COMMANDS {
      if command.match_call(caller.as_str()) {
        return Some(command);
      }
    }
    return None;
  }
  

  It is bad habit to use sentinel values ("".to_string()) instead of proper optionals. You make the code harder to read, because it's less clear where the value is real and where it's a sentinel, and set yourself up for trouble later if sentinel value somehow becomes one of the possible inputs.
  Since an empty string cannot match one of the command-line parameters, you should just early exit if the commad is None.
  The rest of the function can be rewritten using Iterator::find, like this:

  pub fn get(strings: &StringList) -> Option<&Command> {
    let caller: String = Command::get_caller(strings)?;
    COMMANDS.iter().find(|command| command.match_call(&caller))
  }
  

  Note that I have changed the function signature to return Option<&Command>. That's the most natural return type, the caller can clone the result themselves if they want to. Also consider implementing the standard traits (Debug, Copy, Clone, Ord, PartialOrd, Eq, PartialEq, Hash) for your types, unless you have specific reasons to skip some of them. It's just easier down the line.

• Consider using clap for argument parsing. I know, I know, a large crate like clap is a massive overkill for a simple cowsay clone. It will likely take 95% of your compile time. But that's a learning project, right? For real-world apps you're saving yourself and your users a lot of trouble by using industry-standard crates.