0
\$\begingroup\$

I have a directory where there are some script files, some binary files, and some subdirectories. I have tried to write a program which gets names of all files in directory, and excludes if they are directory or binary files. It then reads first line of each file, excludes if "bin" is not there. Then it splits first line by "/" character and gets last string. Hence for first line of "#! /bin/bash", it will get "bash". Then it adds these program names to a HashMap to keep a count and shows it at the end.

Hence, many steps are involved. My program is working, but I feel it can be much optimized as I am just learning Rust.

use std::fs::*;
use std::env;
use std::io::*;
use std::io::prelude::*;
use std::collections::HashMap;
use std::io::{BufReader,BufRead};
use content_inspector::{ContentType, inspect};
use std::path::PathBuf;

fn main() {
    let paths = std::fs::read_dir(".").expect("Not able to read dir.");
    let mut filecount = 0; 
    let mut binarycount =0; 
    let mut dircount =0; 
    let mut progdict = HashMap::new();
    for path in paths{
        println!("=========== PATH ===============");
        let fname = path.unwrap().path(); 
        println!("fname: {}", fname.display()); 
        let pb = PathBuf::from(fname.clone());
        let is_dir = pb.is_dir();
        if is_dir {
            println!("A directory found: {}", pb.display()); 
            dircount += 1; 
            continue; 
            }
        filecount += 1; 
        println!("Not directory; checking further...files checked: {}", filecount); 
        let mut MyBuf = BufReader::new(File::open(fname.clone()).expect("Unable to open file"));
        let mut binary = false; 
        for ch in MyBuf.bytes(){
            if ch.unwrap() > 127{
                println!("BINARY FILE FOUND");
                binary = true; 
                binarycount += 1; 
                break; 
                }
        }
        if binary {continue;}
        println!("Not binary, getting first line."); 
        // else check first line: 
        let file = File::open(fname).expect("cannot open file");
        let file = BufReader::new(file);
        let mut progname: String ="".to_string(); 
        for line in file.lines() {
            let linestr = line.unwrap(); 
            println!("FIRSTLINE: {}", linestr);
            if !linestr.contains("bin"){
                println!("Not script file....");
                break 
            }
            let parts = linestr.split("/").collect::<Vec<&str>>();
            progname = parts[parts.len()-1].trim().to_string(); 
            println!("Program name: {}", progname); 
            break; // we need to see first line only; 
        }
        progdict.entry(progname).and_modify(|x| *x += 1 ).or_insert(1);
    }// end for path in paths; 
    println!("Total files checked: {}; binary files found: {}; dirs found: {}", filecount, binarycount, dircount); 
    for (k,v) in progdict{
        println!("{} :\t\t\t{}", k, v); 
    }
        
}//end main

How can above program be improved? Thanks for your insight.

\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

Firstly, I would advise you to run clippy. It the linter that basically every Rust project uses and it provides very helpful hints. It'll show you unused imports, variable that don't need to be mutable and naming convertions (MyBuf should be my_buf). I won't reproduce everything here, but it also has one other important thing I'd like to mention:

error: this loop never actually loops
  --> src/main.rs:42:9
   |
42 | /         for line in file.lines() {
43 | |             let linestr = line.unwrap();
44 | |             println!("FIRSTLINE: {}", linestr);
45 | |             if !linestr.contains("bin"){
...  |
52 | |             break; // we need to see first line only;
53 | |         }
   | |_________^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#never_loop
   = note: `#[deny(clippy::never_loop)]` on by default
help: if you need the first element of the iterator, try writing
   |
42 |         if let Some(line) = file.lines().next() {
   |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

And it also tells you how to fix it in an idiomatic way.

I would also replace parts[parts.len()-1] by a call to last, which doesn't allocate:

let progname = linestr.split('/').last().unwrap().trim().to_string();

The let mut binary can be replaced of you label the outer loop and use continue 'outer.

The code could also profit from being split into a few smaller functions.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for very good suggestions. \$\endgroup\$
    – rnso
    Commented Apr 24, 2023 at 12:44
1
\$\begingroup\$

I suggest using an error crate like anyhow or color-eyre. This would let you easily handle errors using the ? operator instead of adding .expect() calls everwhere.

I would break up your logic into more functions. In particular, I'd write a function that does something like:

enum FileType {
    Directory,
    Binary,
    Plain,
    Script {
        progname: String
    }
}

fn determine_file_path(path: &Path) -> FileType {
     // figure out the type of path and return a FileType
     // in main you can match on the filetype to handle it
}

Alternatively, you could make a struct to hold the stats you collect.

struct FileStats {
    dircount: u32,
    binarycount: u32,
    filecount: u32,
    progdict: HashMap<String, u32>
}

fn handle_file(state: &mut FileStats, path: &Path) {
   ... 
}

In a function, you have options like early returns or calling other functions to simplify logic, whereas right now you have break and boolean flags operating in a more confusing fashion.

Personally, I'd probably break up into many functions:

fn handle_path(path: &Path) {
   if path.is_dir() {
      handle_directory(path)
   } else {
      handle_file(path)
   }
}

fn handle_file(path: &Path) {
   if is_binary_file(path) {
      ...
   } else {
      ...
   }
}

This block of code:

    let fname = path.unwrap().path(); 
    println!("fname: {}", fname.display()); 
    let pb = PathBuf::from(fname.clone());

Firstly, path() gives you a PathBuf. Then you clone() the PathBuf, getting another copy of it. Then you PathBuf::from create yet another PathBuf. But you didn't need any of that. Just use PathBuf returned from path().

    let mut MyBuf = BufReader::new(File::open(fname.clone()).expect("Unable to open file"));

Instead of calling fname.clone() just pass &fname. You don't need to clone yet another copy of the path, the file apis will happily accept a borrow of a path.

    for line in file.lines() {

You should use the read_line method if you just want to read a single line. Or use file.lines().next() to grab the first element from an iterator. It'll be clearer then break at the end of the loop.

        if !linestr.contains("bin"){

This seems an odd way to check for a script since scripts are typically defined to begin with #!

\$\endgroup\$
6
  • \$\begingroup\$ Some very important points here. Thanks. \$\endgroup\$
    – rnso
    Commented Apr 24, 2023 at 12:44
  • \$\begingroup\$ Regarding clone() vs &, compiler generally recommends clone(). Why is that? \$\endgroup\$
    – rnso
    Commented Apr 24, 2023 at 13:11
  • \$\begingroup\$ Regarding fname from path() vs MyBuf from BufReader, only fname has is_dir() function while bytes() is there in other. Hence, I need to use both. \$\endgroup\$
    – rnso
    Commented Apr 24, 2023 at 13:14
  • \$\begingroup\$ @rnso, the compiler isn't advanced enough to realize that you could do & for paths. \$\endgroup\$ Commented Apr 25, 2023 at 2:19
  • \$\begingroup\$ @rnso, I don't understand your last comment. fname and pb both have is_dir() and are in facts clones. You don't need both. Yes, you need to create a BufReader to get bytes(), but that doesn't mean that you need to clone the path again. \$\endgroup\$ Commented Apr 25, 2023 at 2:21

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.