Lots of code cleanup

- Try to use iterator adapters and collect in various places, where possible. This especially benefits draw_it.
- Try to use `.map` and other similar methods on Options and Results, where possible
- Replaced nearly all clones with reference-based equivalents
- Summarizing nodes by file extension is now much more efficient
- PartialOrd and PartialEq implementations now agree
- Replace #[cfg(...)] function definitions with simpler if cfg!(...) equivelents
- Simplify CLI Values handling by taking advantage of Values::default
- Various spelling corrections in comments
- Add `ColorState` enum to replace bool, for clarity
- Fix tests that break under some detected terminal widths when paths are long
- Use sort_by instead of (sort, reverse)
- Use new `ExtensionNode` struct internally to simplify extension aggregation code

src/dir_walker.rs

@@ -34,14 +34,11 @@ pub fn walk_it(dirs: HashSet<PathBuf>, walk_data: WalkData) -> (Vec<Node>, bool)
     let top_level_nodes: Vec<_> = dirs
         .into_iter()
         .filter_map(|d| {
-            let n = walk(d, &permissions_flag, &walk_data, 0);
+            clean_inodes(
-            match n {
+                walk(d, &permissions_flag, &walk_data, 0)?,
-                Some(n) => {
+                &mut HashSet::new(),
-                    let mut inodes: HashSet<(u64, u64)> = HashSet::new();
+                walk_data.use_apparent_size,
-                    clean_inodes(n, &mut inodes, walk_data.use_apparent_size)
+            )
-                }
-                None => None,
-            }
         })
         .collect();
     (top_level_nodes, permissions_flag.into_inner())
@@ -55,10 +52,9 @@ fn clean_inodes(
 ) -> Option<Node> {
     if !use_apparent_size {
         if let Some(id) = x.inode_device {
-            if inodes.contains(&id) {
+            if !inodes.insert(id) {
                 return None;
             }
-            inodes.insert(id);
         }
     }
 
@@ -133,24 +129,24 @@ fn walk(
 ) -> Option<Node> {
     let mut children = vec![];
 
-    if let Ok(entries) = fs::read_dir(dir.clone()) {
+    if let Ok(entries) = fs::read_dir(&dir) {
         children = entries
             .into_iter()
             .par_bridge()
             .filter_map(|entry| {
                 if let Ok(ref entry) = entry {
                     // uncommenting the below line gives simpler code but
-                    // rayon doesn't parallelise as well giving a 3X performance drop
+                    // rayon doesn't parallelize as well giving a 3X performance drop
                     // hence we unravel the recursion a bit
 
                     // return walk(entry.path(), permissions_flag, ignore_directories, allowed_filesystems, use_apparent_size, by_filecount, ignore_hidden);
 
                     if !ignore_file(entry, walk_data) {
                         if let Ok(data) = entry.file_type() {
-                            if data.is_dir() && !data.is_symlink() {
+                            return if data.is_dir() && !data.is_symlink() {
-                                return walk(entry.path(), permissions_flag, walk_data, depth + 1);
+                                walk(entry.path(), permissions_flag, walk_data, depth + 1)
-                            }
+                            } else {
-                            return build_node(
+                                build_node(
                                     entry.path(),
                                     vec![],
                                     walk_data.filter_regex,
@@ -160,7 +156,8 @@ fn walk(
                                     data.is_file(),
                                     walk_data.by_filecount,
                                     depth,
-                            );
+                                )
+                            };
                         }
                     }
                 } else {
@@ -204,24 +201,26 @@ mod tests {
     }
 
     #[test]
+    #[allow(clippy::redundant_clone)]
     fn test_should_ignore_file() {
         let mut inodes = HashSet::new();
         let n = create_node();
 
         // First time we insert the node
-        assert!(clean_inodes(n.clone(), &mut inodes, false) == Some(n.clone()));
+        assert_eq!(clean_inodes(n.clone(), &mut inodes, false), Some(n.clone()));
 
         // Second time is a duplicate - we ignore it
-        assert!(clean_inodes(n.clone(), &mut inodes, false) == None);
+        assert_eq!(clean_inodes(n.clone(), &mut inodes, false), None);
     }
 
     #[test]
+    #[allow(clippy::redundant_clone)]
     fn test_should_not_ignore_files_if_using_apparent_size() {
         let mut inodes = HashSet::new();
         let n = create_node();
 
         // If using apparent size we include Nodes, even if duplicate inodes
-        assert!(clean_inodes(n.clone(), &mut inodes, true) == Some(n.clone()));
+        assert_eq!(clean_inodes(n.clone(), &mut inodes, true), Some(n.clone()));
-        assert!(clean_inodes(n.clone(), &mut inodes, true) == Some(n.clone()));
+        assert_eq!(clean_inodes(n.clone(), &mut inodes, true), Some(n.clone()));
     }
 }

src/display.rs

@@ -22,7 +22,7 @@ static BLOCKS: [char; 5] = ['█', '▓', '▒', '░', ' '];
 pub struct DisplayData {
     pub short_paths: bool,
     pub is_reversed: bool,
-    pub colors_on: bool,
+    pub colors: ColorState,
     pub by_filecount: bool,
     pub num_chars_needed_on_left_most: usize,
     pub base_size: u64,
@@ -106,26 +106,40 @@ impl DrawData<'_> {
     }
 }
 
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum ColorState {
+    Disabled,
+    Enabled,
+}
+
+impl ColorState {
+    #[inline]
+    #[must_use]
+    fn enabled(self) -> bool {
+        self == ColorState::Enabled
+    }
+}
+
 #[allow(clippy::too_many_arguments)]
 pub fn draw_it(
     use_full_path: bool,
     is_reversed: bool,
-    no_colors: bool,
+    colors: ColorState,
     no_percents: bool,
     terminal_width: usize,
     by_filecount: bool,
-    root_node: DisplayNode,
+    root_node: &DisplayNode,
     iso: bool,
     skip_total: bool,
 ) {
-    let biggest = if skip_total {
+    let biggest = match skip_total {
-        root_node
+        false => root_node,
+        true => root_node
             .get_children_from_node(false)
             .next()
-            .unwrap_or_else(|| root_node.clone())
+            .unwrap_or(root_node),
-    } else {
-        root_node.clone()
     };
+
     let num_chars_needed_on_left_most = if by_filecount {
         let max_size = biggest.size;
         max_size.separate_with_commas().chars().count()
@@ -141,7 +155,7 @@ pub fn draw_it(
     let allowed_width = terminal_width - num_chars_needed_on_left_most - 2;
     let num_indent_chars = 3;
     let longest_string_length =
-        find_longest_dir_name(&root_node, num_indent_chars, allowed_width, !use_full_path);
+        find_longest_dir_name(root_node, num_indent_chars, allowed_width, !use_full_path);
 
     let max_bar_length = if no_percents || longest_string_length + 7 >= allowed_width as usize {
         0
@@ -149,12 +163,12 @@ pub fn draw_it(
         allowed_width as usize - longest_string_length - 7
     };
 
-    let first_size_bar = repeat(BLOCKS[0]).take(max_bar_length).collect::<String>();
+    let first_size_bar = repeat(BLOCKS[0]).take(max_bar_length).collect();
 
     let display_data = DisplayData {
         short_paths: !use_full_path,
         is_reversed,
-        colors_on: !no_colors,
+        colors,
         by_filecount,
         num_chars_needed_on_left_most,
         base_size: biggest.size,
@@ -209,14 +223,14 @@ fn find_longest_dir_name(
         .fold(longest, max)
 }
 
-fn display_node(node: DisplayNode, draw_data: &DrawData, is_biggest: bool, is_last: bool) {
+fn display_node(node: &DisplayNode, draw_data: &DrawData, is_biggest: bool, is_last: bool) {
     // hacky way of working out how deep we are in the tree
     let indent = draw_data.get_new_indent(!node.children.is_empty(), is_last);
     let level = ((indent.chars().count() - 1) / 2) - 1;
-    let bar_text = draw_data.generate_bar(&node, level);
+    let bar_text = draw_data.generate_bar(node, level);
 
     let to_print = format_string(
-        &node,
+        node,
         &*indent,
         &*bar_text,
         is_biggest,
@@ -359,7 +373,7 @@ fn get_pretty_size(node: &DisplayNode, is_biggest: bool, display_data: &DisplayD
     let spaces_to_add = display_data.num_chars_needed_on_left_most - output.chars().count();
     let output = " ".repeat(spaces_to_add) + output.as_str();
 
-    if is_biggest && display_data.colors_on {
+    if is_biggest && display_data.colors.enabled() {
         format!("{}", Red.paint(output))
     } else {
         output
@@ -371,11 +385,11 @@ fn get_pretty_name(
     name_and_padding: String,
     display_data: &DisplayData,
 ) -> String {
-    if display_data.colors_on {
+    if display_data.colors.enabled() {
-        let meta_result = fs::metadata(node.name.clone());
+        let meta_result = fs::metadata(&node.name);
         let directory_color = display_data
             .ls_colors
-            .style_for_path_with_metadata(node.name.clone(), meta_result.as_ref().ok());
+            .style_for_path_with_metadata(&node.name, meta_result.as_ref().ok());
         let ansi_style = directory_color
             .map(Style::to_ansi_term_style)
             .unwrap_or_default();
@@ -411,7 +425,7 @@ mod tests {
         DisplayData {
             short_paths: true,
             is_reversed: false,
-            colors_on: false,
+            colors: ColorState::Disabled,
             by_filecount: false,
             num_chars_needed_on_left_most: 5,
             base_size: 1,

src/display_node.rs

@@ -1,46 +1,24 @@
-use std::cmp::Ordering;
 use std::path::PathBuf;
 
-#[derive(Debug, Eq, Clone)]
+#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Clone)]
 pub struct DisplayNode {
-    pub name: PathBuf, //todo: consider moving to a string?
+    // Note: the order of fields in important here, for PartialEq and PartialOrd
     pub size: u64,
+    pub name: PathBuf, //todo: consider moving to a string?
     pub children: Vec<DisplayNode>,
 }
 
-impl Ord for DisplayNode {
-    fn cmp(&self, other: &Self) -> Ordering {
-        if self.size == other.size {
-            self.name.cmp(&other.name)
-        } else {
-            self.size.cmp(&other.size)
-        }
-    }
-}
-
-impl PartialOrd for DisplayNode {
-    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
-        Some(self.cmp(other))
-    }
-}
-
-impl PartialEq for DisplayNode {
-    fn eq(&self, other: &Self) -> bool {
-        self.name == other.name && self.size == other.size && self.children == other.children
-    }
-}
-
 impl DisplayNode {
     pub fn num_siblings(&self) -> u64 {
         self.children.len() as u64
     }
 
-    pub fn get_children_from_node(&self, is_reversed: bool) -> impl Iterator<Item = DisplayNode> {
+    pub fn get_children_from_node(&self, is_reversed: bool) -> impl Iterator<Item = &DisplayNode> {
         // we box to avoid the clippy lint warning
-        let out: Box<dyn Iterator<Item = DisplayNode>> = if is_reversed {
+        let out: Box<dyn Iterator<Item = &DisplayNode>> = if is_reversed {
-            Box::new(self.children.clone().into_iter().rev())
+            Box::new(self.children.iter().rev())
         } else {
-            Box::new(self.children.clone().into_iter())
+            Box::new(self.children.iter())
         };
         out
     }

src/filter.rs

@@ -3,6 +3,8 @@ use crate::node::Node;
 use std::collections::BinaryHeap;
 use std::collections::HashMap;
 use std::collections::HashSet;
+use std::ffi::OsStr;
+use std::path::Path;
 use std::path::PathBuf;
 
 pub fn get_biggest(
@@ -21,14 +23,14 @@ pub fn get_biggest(
     let root = get_new_root(top_level_nodes);
     let mut allowed_nodes = HashSet::new();
 
-    allowed_nodes.insert(&root.name);
+    allowed_nodes.insert(root.name.as_path());
     heap = add_children(using_a_filter, &root, depth, heap);
 
     for _ in number_top_level_nodes..n {
         let line = heap.pop();
         match line {
             Some(line) => {
-                allowed_nodes.insert(&line.name);
+                allowed_nodes.insert(line.name.as_path());
                 heap = add_children(using_a_filter, line, depth, heap);
             }
             None => break,
@@ -37,33 +39,59 @@ pub fn get_biggest(
     recursive_rebuilder(&allowed_nodes, &root)
 }
 
-pub fn get_all_file_types(top_level_nodes: Vec<Node>, n: usize) -> Option<DisplayNode> {
+#[derive(PartialEq, Eq, PartialOrd, Ord)]
-    let mut map: HashMap<String, DisplayNode> = HashMap::new();
+struct ExtensionNode<'a> {
-    build_by_all_file_types(top_level_nodes, &mut map);
+    size: u64,
-    let mut by_types: Vec<DisplayNode> = map.into_iter().map(|(_k, v)| v).collect();
+    extension: Option<&'a OsStr>,
-    by_types.sort();
+}
-    by_types.reverse();
 
-    let displayed = if by_types.len() <= n {
+pub fn get_all_file_types(top_level_nodes: &[Node], n: usize) -> Option<DisplayNode> {
-        by_types
+    let ext_nodes = {
-    } else {
+        let mut extension_cumulative_sizes = HashMap::new();
-        let (displayed, rest) = by_types.split_at(if n > 1 { n - 1 } else { 1 });
+        build_by_all_file_types(top_level_nodes, &mut extension_cumulative_sizes);
-        let remaining = DisplayNode {
+
-            name: PathBuf::from("(others)"),
+        let mut extension_cumulative_sizes: Vec<ExtensionNode<'_>> = extension_cumulative_sizes
-            size: rest.iter().map(|a| a.size).sum(),
+            .iter()
+            .map(|(&extension, &size)| ExtensionNode { extension, size })
+            .collect();
+
+        extension_cumulative_sizes.sort_by(|lhs, rhs| lhs.cmp(rhs).reverse());
+
+        extension_cumulative_sizes
+    };
+
+    let mut ext_nodes_iter = ext_nodes.iter();
+
+    // First, collect the first N - 1 nodes...
+    let mut displayed: Vec<DisplayNode> = ext_nodes_iter
+        .by_ref()
+        .take(if n > 1 { n - 1 } else { 1 })
+        .map(|node| DisplayNode {
+            name: PathBuf::from(
+                node.extension
+                    .map(|ext| format!(".{}", ext.to_string_lossy()))
+                    .unwrap_or_else(|| "(no extension)".to_owned()),
+            ),
+            size: node.size,
             children: vec![],
-        };
+        })
+        .collect();
 
-        let mut displayed = displayed.to_vec();
+    // ...then, aggregate the remaining nodes (if any) into a single  "(others)" node
-        displayed.push(remaining);
+    if ext_nodes_iter.len() > 0 {
-        displayed
+        displayed.push(DisplayNode {
-    };
+            name: PathBuf::from("(others)"),
+            size: ext_nodes_iter.map(|node| node.size).sum(),
+            children: vec![],
+        });
+    }
 
     let result = DisplayNode {
         name: PathBuf::from("(total)"),
-        size: displayed.iter().map(|a| a.size).sum(),
+        size: displayed.iter().map(|node| node.size).sum(),
         children: displayed,
     };
+
     Some(result)
 }
 
@@ -74,44 +102,35 @@ fn add_children<'a>(
     mut heap: BinaryHeap<&'a Node>,
 ) -> BinaryHeap<&'a Node> {
     if depth > file_or_folder.depth {
-        if using_a_filter {
+        heap.extend(
-            file_or_folder.children.iter().for_each(|c| {
+            file_or_folder
-                if c.name.is_file() || c.size > 0 {
+                .children
-                    heap.push(c)
+                .iter()
-                }
+                .filter(|c| !using_a_filter || c.name.is_file() || c.size > 0),
-            });
+        )
-        } else {
-            file_or_folder.children.iter().for_each(|c| heap.push(c));
-        }
     }
     heap
 }
 
-fn build_by_all_file_types(top_level_nodes: Vec<Node>, counter: &mut HashMap<String, DisplayNode>) {
+fn build_by_all_file_types<'a>(
+    top_level_nodes: &'a [Node],
+    counter: &mut HashMap<Option<&'a OsStr>, u64>,
+) {
     for node in top_level_nodes {
         if node.name.is_file() {
             let ext = node.name.extension();
-            let key: String = match ext {
+            let cumulative_size = counter.entry(ext).or_default();
-                Some(e) => ".".to_string() + &e.to_string_lossy(),
+            *cumulative_size += node.size;
-                None => "(no extension)".into(),
-            };
-            let mut display_node = counter.entry(key.clone()).or_insert(DisplayNode {
-                name: PathBuf::from(key),
-                size: 0,
-                children: vec![],
-            });
-            display_node.size += node.size;
         }
-        build_by_all_file_types(node.children, counter)
+        build_by_all_file_types(&node.children, counter)
     }
 }
 
 fn get_new_root(top_level_nodes: Vec<Node>) -> Node {
     if top_level_nodes.len() > 1 {
-        let total_size = top_level_nodes.iter().map(|node| node.size).sum();
         Node {
             name: PathBuf::from("(total)"),
-            size: total_size,
+            size: top_level_nodes.iter().map(|node| node.size).sum(),
             children: top_level_nodes,
             inode_device: None,
             depth: 0,
@@ -121,27 +140,19 @@ fn get_new_root(top_level_nodes: Vec<Node>) -> Node {
     }
 }
 
-fn recursive_rebuilder<'a>(
+fn recursive_rebuilder(allowed_nodes: &HashSet<&Path>, current: &Node) -> Option<DisplayNode> {
-    allowed_nodes: &'a HashSet<&PathBuf>,
-    current: &Node,
-) -> Option<DisplayNode> {
     let mut new_children: Vec<_> = current
         .children
         .iter()
-        .filter_map(|c| {
+        .filter(|c| allowed_nodes.contains(c.name.as_path()))
-            if allowed_nodes.contains(&c.name) {
+        .filter_map(|c| recursive_rebuilder(allowed_nodes, c))
-                recursive_rebuilder(allowed_nodes, c)
-            } else {
-                None
-            }
-        })
         .collect();
-    new_children.sort();
+
-    new_children.reverse();
+    new_children.sort_by(|lhs, rhs| lhs.cmp(rhs).reverse());
-    let newnode = DisplayNode {
+
+    Some(DisplayNode {
         name: current.name.clone(),
         size: current.size,
         children: new_children,
-    };
+    })
-    Some(newnode)
 }

src/main.rs

@@ -6,7 +6,7 @@ extern crate unicode_width;
 use std::collections::HashSet;
 use std::process;
 
-use self::display::draw_it;
+use self::display::{draw_it, ColorState};
 use clap::{crate_version, Arg};
 use clap::{Command, Values};
 use dir_walker::{walk_it, WalkData};
@@ -29,73 +29,72 @@ mod utils;
 static DEFAULT_NUMBER_OF_LINES: usize = 30;
 static DEFAULT_TERMINAL_WIDTH: usize = 80;
 
+/// `ansi_term::enable_ansi_support` only exists on Windows; this wrapper
+/// function makes it available on all platforms
+#[inline]
+fn enable_ansi_support() -> Result<(), i32> {
     #[cfg(windows)]
-fn init_color(no_color: bool) -> bool {
+    {
+        ansi_term::enable_ansi_support()
+    }
+
+    #[cfg(not(windows))]
+    {
+        Ok(())
+    }
+}
+
+fn init_color(color: ColorState) -> ColorState {
+    match color {
         // If no color is already set do not print a warning message
-    if no_color {
+        ColorState::Disabled => ColorState::Disabled,
-        true
+
-    } else {
         // Required for windows 10
         // Fails to resolve for windows 8 so disable color
-        match ansi_term::enable_ansi_support() {
+        ColorState::Enabled => match enable_ansi_support() {
-            Ok(_) => no_color,
+            Ok(()) => ColorState::Enabled,
             Err(_) => {
                 eprintln!(
                     "This version of Windows does not support ANSI colors, setting no_color flag"
                 );
-                true
+                ColorState::Disabled
             }
+        },
     }
 }
-}
-
-#[cfg(not(windows))]
-fn init_color(no_color: bool) -> bool {
-    no_color
-}
 
 fn get_height_of_terminal() -> usize {
+    // Simplify once https://github.com/eminence/terminal-size/pull/41 is
+    // merged
+    terminal_size()
         // Windows CI runners detect a terminal height of 0
-    if let Some((Width(_w), Height(h))) = terminal_size() {
+        .map(|(_, Height(h))| max(h as usize, DEFAULT_NUMBER_OF_LINES))
-        max(h as usize, DEFAULT_NUMBER_OF_LINES) - 10
+        .unwrap_or(DEFAULT_NUMBER_OF_LINES)
-    } else {
+        - 10
-        DEFAULT_NUMBER_OF_LINES - 10
-    }
 }
 
-#[cfg(windows)]
 fn get_width_of_terminal() -> usize {
+    // Simplify once https://github.com/eminence/terminal-size/pull/41 is
+    // merged
+    terminal_size()
+        .map(|(Width(w), _)| match cfg!(windows) {
             // Windows CI runners detect a very low terminal width
-    if let Some((Width(w), Height(_h))) = terminal_size() {
+            true => max(w as usize, DEFAULT_TERMINAL_WIDTH),
-        max(w as usize, DEFAULT_TERMINAL_WIDTH)
+            false => w as usize,
-    } else {
+        })
-        DEFAULT_TERMINAL_WIDTH
+        .unwrap_or(DEFAULT_TERMINAL_WIDTH)
-    }
-}
-
-#[cfg(not(windows))]
-fn get_width_of_terminal() -> usize {
-    if let Some((Width(w), Height(_h))) = terminal_size() {
-        w as usize
-    } else {
-        DEFAULT_TERMINAL_WIDTH
-    }
 }
 
 fn get_regex_value(maybe_value: Option<Values>) -> Vec<Regex> {
-    let mut result = vec![];
+    maybe_value
-    if let Some(v) = maybe_value {
+        .unwrap_or_default()
-        for reg in v {
+        .map(|reg| {
-            match Regex::new(reg) {
+            Regex::new(reg).unwrap_or_else(|err| {
-                Ok(r) => result.push(r),
+                eprintln!("Ignoring bad value for regex {:?}", err);
-                Err(e) => {
+                process::exit(1)
-                    eprintln!("Ignoring bad value for regex {:?}", e);
+            })
-                    process::exit(1);
+        })
-                }
+        .collect()
-            }
-        }
-    }
-    result
 }
 
 fn main() {
@@ -235,18 +234,15 @@ fn main() {
     let filter_regexs = get_regex_value(options.values_of("filter"));
     let invert_filter_regexs = get_regex_value(options.values_of("invert_filter"));
 
-    let terminal_width = match options.value_of_t("width") {
+    let terminal_width = options
-        Ok(v) => v,
+        .value_of_t("width")
-        Err(_) => get_width_of_terminal(),
+        .unwrap_or_else(|_| get_width_of_terminal());
-    };
 
-    let depth = match options.value_of_t("depth") {
+    let depth = options.value_of_t("depth").unwrap_or_else(|_| {
-        Ok(v) => v,
-        Err(_) => {
         eprintln!("Ignoring bad value for depth");
         usize::MAX
-        }
+    });
-    };
+
     // If depth is set we set the default number_of_lines to be max
     // instead of screen height
     let default_height = if depth != usize::MAX {
@@ -255,40 +251,36 @@ fn main() {
         get_height_of_terminal()
     };
 
-    let number_of_lines = match options.value_of("number_of_lines") {
+    let number_of_lines = options
-        Some(v) => match v.parse::<usize>() {
+        .value_of("number_of_lines")
-            Ok(num_lines) => num_lines,
+        .and_then(|v| {
-            Err(_) => {
+            v.parse()
-                eprintln!("Ignoring bad value for number_of_lines");
+                .map_err(|_| eprintln!("Ignoring bad value for number_of_lines"))
-                default_height
+                .ok()
-            }
+        })
-        },
+        .unwrap_or(default_height);
-        None => default_height,
-    };
 
-    let no_colors = init_color(options.is_present("no_colors"));
+    let colors = init_color(match options.is_present("no_colors") {
+        false => ColorState::Enabled,
+        true => ColorState::Disabled,
+    });
     let use_apparent_size = options.is_present("display_apparent_size");
-    let ignore_directories: Vec<PathBuf> = options
+    let ignore_directories = options
         .values_of("ignore_directory")
-        .map(|i| i.map(PathBuf::from).collect())
+        .unwrap_or_default()
-        .unwrap_or_default();
+        .map(PathBuf::from);
 
     let by_filecount = options.is_present("by_filecount");
     let ignore_hidden = options.is_present("ignore_hidden");
     let limit_filesystem = options.is_present("limit_filesystem");
 
     let simplified_dirs = simplify_dir_names(target_dirs);
-    let allowed_filesystems = {
+    let allowed_filesystems = limit_filesystem
-        if limit_filesystem {
+        .then(|| get_filesystem_devices(simplified_dirs.iter()))
-            get_filesystem_devices(simplified_dirs.iter())
+        .unwrap_or_default();
-        } else {
-            HashSet::new()
-        }
-    };
 
     let ignored_full_path: HashSet<PathBuf> = ignore_directories
-        .into_iter()
+        .flat_map(|x| simplified_dirs.iter().map(move |d| d.join(&x)))
-        .flat_map(|x| simplified_dirs.iter().map(move |d| d.join(x.clone())))
         .collect();
 
     let walk_data = WalkData {
@@ -308,34 +300,31 @@ fn main() {
 
     let (top_level_nodes, has_errors) = walk_it(simplified_dirs, walk_data);
 
-    let tree = {
+    let tree = match summarize_file_types {
-        match (depth, summarize_file_types) {
+        true => get_all_file_types(&top_level_nodes, number_of_lines),
-            (_, true) => get_all_file_types(top_level_nodes, number_of_lines),
+        false => get_biggest(
-            (depth, _) => get_biggest(
             top_level_nodes,
             number_of_lines,
             depth,
-                options.values_of("filter").is_some()
+            options.values_of("filter").is_some() || options.value_of("invert_filter").is_some(),
-                    || options.value_of("invert_filter").is_some(),
         ),
-        }
     };
 
     if has_errors {
         eprintln!("Did not have permissions for all directories");
     }
-    match tree {
+
-        None => {}
+    if let Some(root_node) = tree {
-        Some(root_node) => draw_it(
+        draw_it(
             options.is_present("display_full_paths"),
             !options.is_present("reverse"),
-            no_colors,
+            colors,
             options.is_present("no_bars"),
             terminal_width,
             by_filecount,
-            root_node,
+            &root_node,
             options.is_present("iso"),
             options.is_present("skip_total"),
-        ),
+        )
     }
 }

src/node.rs

@@ -27,8 +27,7 @@ pub fn build_node(
     by_filecount: bool,
     depth: usize,
 ) -> Option<Node> {
-    match get_metadata(&dir, use_apparent_size) {
+    get_metadata(&dir, use_apparent_size).map(|data| {
-        Some(data) => {
         let inode_device = if is_symlink && !use_apparent_size {
             None
         } else {
@@ -47,17 +46,15 @@ pub fn build_node(
             data.0
         };
 
-            Some(Node {
+        Node {
             name: dir,
             size,
             children,
             inode_device,
             depth,
+        }
     })
 }
-        None => None,
-    }
-}
 
 impl PartialEq for Node {
     fn eq(&self, other: &Self) -> bool {
@@ -67,11 +64,10 @@ impl PartialEq for Node {
 
 impl Ord for Node {
     fn cmp(&self, other: &Self) -> Ordering {
-        if self.size == other.size {
+        self.size
-            self.name.cmp(&other.name)
+            .cmp(&other.size)
-        } else {
+            .then_with(|| self.name.cmp(&other.name))
-            self.size.cmp(&other.size)
+            .then_with(|| self.children.cmp(&other.children))
-        }
     }
 }
 

src/platform.rs

@@ -5,7 +5,7 @@ use std::path::Path;
 
 #[cfg(target_family = "unix")]
 fn get_block_size() -> u64 {
-    // All os specific implementations of MetatdataExt seem to define a block as 512 bytes
+    // All os specific implementations of MetadataExt seem to define a block as 512 bytes
     // https://doc.rust-lang.org/std/os/linux/fs/trait.MetadataExt.html#tymethod.st_blocks
     512
 }
@@ -105,12 +105,12 @@ pub fn get_metadata(d: &Path, _use_apparent_size: bool) -> Option<(u64, Option<(
     use std::os::windows::fs::MetadataExt;
     match d.metadata() {
         Ok(ref md) => {
-            const FILE_ATTRIBUTE_ARCHIVE: u32 = 0x20u32;
+            const FILE_ATTRIBUTE_ARCHIVE: u32 = 0x20;
-            const FILE_ATTRIBUTE_READONLY: u32 = 0x1u32;
+            const FILE_ATTRIBUTE_READONLY: u32 = 0x01;
-            const FILE_ATTRIBUTE_HIDDEN: u32 = 0x2u32;
+            const FILE_ATTRIBUTE_HIDDEN: u32 = 0x02;
-            const FILE_ATTRIBUTE_SYSTEM: u32 = 0x4u32;
+            const FILE_ATTRIBUTE_SYSTEM: u32 = 0x04;
-            const FILE_ATTRIBUTE_NORMAL: u32 = 0x80u32;
+            const FILE_ATTRIBUTE_NORMAL: u32 = 0x80;
-            const FILE_ATTRIBUTE_DIRECTORY: u32 = 0x10u32;
+            const FILE_ATTRIBUTE_DIRECTORY: u32 = 0x10;
 
             let attr_filtered = md.file_attributes()
                 & !(FILE_ATTRIBUTE_HIDDEN | FILE_ATTRIBUTE_READONLY | FILE_ATTRIBUTE_SYSTEM);

src/utils.rs

@@ -7,6 +7,7 @@ use regex::Regex;
 
 pub fn simplify_dir_names<P: AsRef<Path>>(filenames: Vec<P>) -> HashSet<PathBuf> {
     let mut top_level_names: HashSet<PathBuf> = HashSet::with_capacity(filenames.len());
+
     for t in filenames {
         let top_level_name = normalize_path(t);
         let mut can_add = true;
@@ -26,6 +27,7 @@ pub fn simplify_dir_names<P: AsRef<Path>>(filenames: Vec<P>) -> HashSet<PathBuf>
             top_level_names.insert(top_level_name);
         }
     }
+
     top_level_names
 }
 
@@ -33,14 +35,9 @@ pub fn get_filesystem_devices<'a, P: IntoIterator<Item = &'a PathBuf>>(paths: P)
     // Gets the device ids for the filesystems which are used by the argument paths
     paths
         .into_iter()
-        .filter_map(|p| {
+        .filter_map(|p| match get_metadata(p, false) {
-            let meta = get_metadata(p, false);
+            Some((_size, Some((_id, dev)))) => Some(dev),
-
+            _ => None,
-            if let Some((_size, Some((_id, dev)))) = meta {
-                Some(dev)
-            } else {
-                None
-            }
         })
         .collect()
 }
@@ -52,7 +49,7 @@ pub fn normalize_path<P: AsRef<Path>>(path: P) -> PathBuf {
     // 3. removing trailing extra separators and '.' ("current directory") path segments
     // * `Path.components()` does all the above work; ref: <https://doc.rust-lang.org/std/path/struct.Path.html#method.components>
     // 4. changing to os preferred separator (automatically done by recollecting components back into a PathBuf)
-    path.as_ref().components().collect::<PathBuf>()
+    path.as_ref().components().collect()
 }
 
 pub fn is_filtered_out_due_to_regex(filter_regex: &[Regex], dir: &Path) -> bool {

tests/test_exact_output.rs

@@ -20,14 +20,11 @@ fn copy_test_data(dir: &str) {
     // First remove the existing directory - just incase it is there and has incorrect data
     let last_slash = dir.rfind('/').unwrap();
     let last_part_of_dir = dir.chars().skip(last_slash).collect::<String>();
-    match Command::new("rm")
+    let _ = Command::new("rm")
         .arg("-rf")
         .arg("/tmp/".to_owned() + &*last_part_of_dir)
-        .ok()
+        .ok();
-    {
+
-        Ok(_) => {}
-        Err(_) => {}
-    };
     match Command::new("cp").arg("-r").arg(dir).arg("/tmp/").ok() {
         Ok(_) => {}
         Err(err) => {
@@ -48,14 +45,14 @@ fn exact_output_test<T: AsRef<OsStr>>(valid_outputs: Vec<String>, command_args:
     initialize();
 
     let mut a = &mut Command::cargo_bin("dust").unwrap();
+
     for p in command_args {
         a = a.arg(p);
     }
-    let output: String = str::from_utf8(&a.unwrap().stdout).unwrap().into();
 
-    assert!(valid_outputs
+    let output = str::from_utf8(&a.unwrap().stdout).unwrap().to_owned();
-        .iter()
+
-        .fold(false, |sum, i| sum || output.contains(i)));
+    assert!(valid_outputs.iter().any(|i| output.contains(i)));
 }
 
 // "windows" result data can vary by host (size seems to be variable by one byte); fix code vs test and re-enable
@@ -129,7 +126,7 @@ fn main_output_long_paths() -> Vec<String> {
     vec![mac_and_some_linux, ubuntu]
 }
 
-// Check against directories and files whos names are substrings of each other
+// Check against directories and files whose names are substrings of each other
 #[cfg_attr(target_os = "windows", ignore)]
 #[test]
 pub fn test_substring_of_names_and_long_names() {

tests/test_flags.rs

@@ -67,7 +67,7 @@ pub fn test_d_flag_works_and_still_recurses_down() {
     assert!(output.contains("4 ┌─┴ test_dir2"));
 }
 
-// Check against directories and files whos names are substrings of each other
+// Check against directories and files whose names are substrings of each other
 #[test]
 pub fn test_ignore_dir() {
     let output = build_command(vec!["-c", "-X", "dir_substring", "tests/test_dir2/"]);

tests/tests_symlinks.rs

@@ -39,9 +39,12 @@ pub fn test_soft_sym_link() {
     let a = format!("─┴ {}", dir_s);
 
     let mut cmd = Command::cargo_bin("dust").unwrap();
-    // Mac test runners create long filenames in tmp directories
     let output = cmd
-        .args(["-p", "-c", "-s", "-w 999", dir_s])
+        .arg("-p")
+        .arg("-c")
+        .arg("-s")
+        .args(["-w", "999"])
+        .arg(dir_s)
         .unwrap()
         .stdout;
 
@@ -72,8 +75,13 @@ pub fn test_hard_sym_link() {
     let dirs_output = format!("─┴ {}", dir_s);
 
     let mut cmd = Command::cargo_bin("dust").unwrap();
-    // Mac test runners create long filenames in tmp directories
+    let output = cmd
-    let output = cmd.args(["-p", "-c", "-w 999", dir_s]).unwrap().stdout;
+        .arg("-p")
+        .arg("-c")
+        .args(["-w", "999"])
+        .arg(dir_s)
+        .unwrap()
+        .stdout;
 
     // The link should not appear in the output because multiple inodes are now ordered
     // then filtered.