From 5823d36e845e405c17a6ddd1e24d52fa5c06aa92 Mon Sep 17 00:00:00 2001
From: Paul Osborne <osbpau@gmail.com>
Date: Tue, 3 Mar 2015 23:51:54 -0600
Subject: [PATCH] Updates based on code review by @nastevens
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This addresses the following feeback:

Your code looks really good, and very Rustic. I only have a few comments:
· In ProcessRecord you use isize for the pid and ppid. These aren’t really sizes, so I’d go with i32
· Line 102: since all you’re doing is panic!() on Err, you can just use .unwrap()
· Line 107: you could use “filter_map” instead of filter and then “collect” the results. This eliminates the need for “mut records” on line 98
· Line 126: use “filter_map”
· I’d add Cargo support
· You could consider using a map type containing vector types for storing the ppid->pid translation. That would improve performance since the record list wouldn’t getting iterated over multiple times. Unfortunately Rust doesn’t have a multimap yet (see https://github.com/rust-lang/rfcs/issues/784), otherwise you could just use that!

Using a map type for the lookup is not supported with this but will
be evaluated.
---
 pstree.rs | 46 ++++++++++++++++++++--------------------------
 1 file changed, 20 insertions(+), 26 deletions(-)

diff --git a/pstree.rs b/pstree.rs
index d42783c..0e0110d 100644
--- a/pstree.rs
+++ b/pstree.rs
@@ -39,8 +39,8 @@ use std::old_io::BufferedReader;
 #[derive(Clone,Debug)]
 struct ProcessRecord {
     name: String,
-    pid: isize,
-    ppid: isize,
+    pid: i32,
+    ppid: i32,
 }
 
 #[derive(Clone,Debug)]
@@ -65,8 +65,8 @@ impl ProcessTreeNode {
 // Given a status file path, return a hashmap with the following form:
 // pid -> ProcessRecord
 fn get_process_record(status_path: &Path) -> Option<ProcessRecord> {
-    let mut pid : Option<isize> = None;
-    let mut ppid : Option<isize> = None;
+    let mut pid : Option<i32> = None;
+    let mut ppid : Option<i32> = None;
     let mut name : Option<String> = None;
 
     let mut status_file = BufferedReader::new(File::open(status_path));
@@ -95,41 +95,35 @@ fn get_process_record(status_path: &Path) -> Option<ProcessRecord> {
 
 // build a simple struct (ProcessRecord) for each process
 fn get_process_records() -> Vec<ProcessRecord> {
-    let mut records : Vec<ProcessRecord> = Vec::new();
     let proc_directory = Path::new("/proc");
 
     // find potential process directories under /proc
-    let proc_directory_contents = match fs::readdir(&proc_directory) {
-        Err(why) => panic!("{}", why.desc),
-        Ok(res) => res
-    };
-
-    for entry in proc_directory_contents.iter().filter(|entry| entry.is_dir()) {
-        let status_path = entry.join("status");
-        if status_path.exists() {
-            match get_process_record(&status_path) {
-                Some(record) => {
-                    records.push(record)
-                },
-                None => (),
+    let proc_directory_contents = fs::readdir(&proc_directory).unwrap();
+    proc_directory_contents.iter().filter_map(|entry| {
+        if entry.is_dir() {
+            let status_path = entry.join("status");
+            if status_path.exists() {
+                return get_process_record(&status_path)
             }
         }
-    }
-    records
+        None
+    }).collect()
 }
 
 fn populate_node(node : &mut ProcessTreeNode, records: &Vec<ProcessRecord>) {
     // populate the node by finding its children... recursively
     let pid = node.record.pid; // avoid binding node as immutable in closure
     node.children.extend(
-        records.iter()
-            .filter(|record| record.ppid == pid)
-            .map(|record| {
+        records.iter().filter_map(|record| {
+            if record.ppid == pid {
                 let mut child = ProcessTreeNode::new(record);
                 populate_node(&mut child, records);
-                child
-            })
-    );
+                Some(child)
+            } else {
+                None
+            }
+        })
+    )
 }
 
 fn build_process_tree() -> ProcessTree {