Checking which programs are used for scripting

Question

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.

Answer 1

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 #!

Answer 2

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.