I implemented ls -l in Rust as a beginners project - Is anyone willing to improve my code?

[Lesso]

Hello again,

I wrote a small implementation of ls -l or ll in Rust, as my second beginners program.
I got it to work just fine, but of course I'm not too sure if my approach is good.

Usually this kind of request is considered a little bit "rude", since it's not a specific question, but I hope you could maybe still try to help me, by giving me general adivce on what to improve.

If you still consider this thread a violation of rules or just plain rude, please close it.

First off, here is the code.
And here are my dependencies in Cargo.toml:

[dependencies]
chrono = "0.4.7"
users = "0.9"

Of course I'm happy about any kind of advice to improve my code.

Thanks already in advance!

[hellow]

• chrono = "*" is a big no go. You should specify the version you use, because elsewise it will very certainly.
• use std::os::macos::fs::MetadataExt; this makes it not possible for me to use, because I don't have a mac. Make that more open, don't use OS specific traits.

[kornel]

@hellow

Make that more open, don’t use OS specific traits.

Or rather use them, but hidden behind #[cfg(target_os = "macos")].

[Lesso]

@hellow, thanks for your input.
I updated my Cargo.toml. The source code (changed the link) now generifies the use statement to target unix instead of macos.
I hope that's okay.
The code does not execute under Windows though, since as far as I understood, the PermissionsExt struct is not supported by none-unix systems.

[Yandros]

    let mut permissions : [char; 10] = unsafe {
        let mut array : [char; 10] = mem::uninitialized();
        for (_i,e) in array.iter_mut().enumerate() {
            ptr::write(e, '-');
        }
        array
    };

Avoid using unsafe, especially if you are just beginning; in this case there is a much simpler way to do the same thing:

let mut permissions: [char; 10] = ['-'; 10];

---

I have taken your file and tweaked it here and there, resulting in:

https://gist.github.com/rust-play/3bb4c40b84858e14f9b1e72032c28704

• I have changed unwraps and expects into ?, to make errors just bubble up. On the other hand, I have io::Error as the error type, which sadly does not keep track of, for instance, the path to the thing that lead to an error, resulting in very unhelpful error messages

  • A quick and dirty solution for this (a binary) is using String (or something akin to it) as the error type:

    foo()
        .map_err(|err| format!("My good-looking error message ({})", err))?
    

• Since you wanted your map to be sorted, I have replaced HashMap by BTreeMap;

• I have moved auxiliary functions inside the functions using them (if you then need to access one from another function, you can just move it back out, maybe in its own mod): this slightly improves readability;

• I have removed the 6-args constructor for FileStats: with so many arguments, the struct literal syntax is more readable than a function call. For a "readable constructor", see the builder pattern with ::derive_builder to reduce the boilerplate.

• I have replaced boolean negations using ! by a .not() call. I find this more readable, but that's just my opinion.

• Instead of using an array ['d', '-', 'r', 'w', 'x'] and indexing it with numbers, using an enum or just some constants is more readable:

  fn build_permissions (is_dir: bool, mode: u32) -> String
  {
      #[allow(non_snake_case)]
      mod Perms {
          pub const DIR    : char = 'd';
          pub const NONE   : char = '-';
          pub const READ   : char = 'r';
          pub const WRITE  : char = 'w';
          pub const EXECUTE: char = 'x';
      }
  
      let mut mask = 1 << 9;
      iter::once(
          if is_dir { Perms::DIR } else { Perms::NONE }
      ).chain({
          const RWX: [char; 3] =
              [Perms::READ, Perms::WRITE, Perms::EXECUTE]
          ;
          (0 .. 9).map(|i| {
              mask >>= 1;
              if mode & mask != 0 {
                  RWX[i % 3]
              } else {
                  Perms::NONE
              }
          })
      }).collect()
  }
  

  • The function body, on the other hand, may not be very readable for a beginner.
    It is using Iterator adaptors.
    Getting the hang of it is what leads to writing both safe and performant code

• I have used padding with the format strings instead of \t chars;

• and other minor tweaks to get an output closer to ls'

---

Still far from perfect, but it should be enough to show you some Rust idioms in action

[Lesso]

Wow, that's more than I expected!
Thanks a lot. This helped so much!

I really appreciate your work.
I will work through all of your optimizations in the next coming days