refactor finder logic

src/finder.rs

@@ -1,16 +1,44 @@
+use error::*;
+#[cfg(windows)]
+use helper::has_executable_extension;
 use std::env;
 use std::ffi::OsStr;
 #[cfg(windows)]
 use std::ffi::OsString;
+use std::iter;
 use std::path::{Path, PathBuf};
-use error::*;
-#[cfg(windows)]
-use helper::has_executable_extension;
 
 pub trait Checker {
     fn is_valid(&self, path: &Path) -> bool;
 }
 
+trait PathExt {
+    fn has_separator(&self) -> bool;
+
+    fn to_absolute<P>(self, cwd: P) -> PathBuf
+    where
+        P: AsRef<Path>;
+}
+
+impl PathExt for PathBuf {
+    fn has_separator(&self) -> bool {
+        self.components().count() > 1
+    }
+
+    fn to_absolute<P>(self, cwd: P) -> PathBuf
+    where
+        P: AsRef<Path>,
+    {
+        if self.is_absolute() {
+            self
+        } else {
+            let mut new_path = PathBuf::from(cwd.as_ref());
+            new_path.push(self);
+            new_path
+        }
+    }
+}
+
 pub struct Finder;
 
 impl Finder {
@@ -31,75 +59,97 @@ impl Finder {
         V: AsRef<Path>,
     {
         let path = PathBuf::from(&binary_name);
-        // Does it have a path separator?
-        if path.components().count() > 1 {
-            if path.is_absolute() {
-                self.check_with_exe_extension(path, binary_checker)
-                    .ok_or(ErrorKind::BadAbsolutePath.into())
-            } else {
-                // Try to make it absolute.
-                let mut new_path = PathBuf::from(cwd.as_ref());
-                new_path.push(path);
-                self.check_with_exe_extension(new_path, binary_checker)
-                    .ok_or(ErrorKind::BadRelativePath.into())
-            }
+
+        let binary_path_candidates: Box<dyn Iterator<Item = _>> = if path.has_separator() {
+            // Search binary in cwd if the path have a path separator.
+            let candidates = Self::cwd_search_candidates(path, cwd).into_iter();
+            Box::new(candidates)
         } else {
-            // No separator, look it up in `paths`.
-            paths
-                .and_then(|paths| {
-                    env::split_paths(&paths)
-                        .map(|p| p.join(binary_name.as_ref()))
-                        .map(|p| self.check_with_exe_extension(p, binary_checker))
-                        .skip_while(|res| res.is_none())
-                        .next()
-                })
-                .map(|res| res.unwrap())
-                .ok_or(ErrorKind::CannotFindBinaryPath.into())
+            // Search binary in PATHs(defined in environment variable).
+            let p = paths.ok_or(ErrorKind::CannotFindBinaryPath)?;
+            let paths: Vec<_> = env::split_paths(&p).collect();
+
+            let candidates = Self::path_search_candidates(path, paths).into_iter();
+
+            Box::new(candidates)
+        };
+
+        for p in binary_path_candidates {
+            // find a valid binary
+            if binary_checker.is_valid(&p) {
+                return Ok(p);
+            }
         }
+
+        // can't find any binary
+        return Err(ErrorKind::CannotFindBinaryPath.into());
+    }
+
+    fn cwd_search_candidates<C>(binary_name: PathBuf, cwd: C) -> impl IntoIterator<Item = PathBuf>
+    where
+        C: AsRef<Path>,
+    {
+        let path = binary_name.to_absolute(cwd);
+
+        Self::append_extension(iter::once(path))
+    }
+
+    fn path_search_candidates<P>(
+        binary_name: PathBuf,
+        paths: P,
+    ) -> impl IntoIterator<Item = PathBuf>
+    where
+        P: IntoIterator<Item = PathBuf>,
+    {
+        let new_paths = paths.into_iter().map(move |p| p.join(binary_name.clone()));
+
+        Self::append_extension(new_paths)
     }
 
     #[cfg(unix)]
-    /// Check if given path with platform specification is valid
-    pub fn check_with_exe_extension<T: AsRef<Path>>(&self, path: T, binary_checker: &Checker) -> Option<PathBuf> {
-        if binary_checker.is_valid(path.as_ref()) {
-            Some(path.as_ref().to_path_buf())
-        } else {
-            None
-        }
+    fn append_extension<P>(paths: P) -> impl IntoIterator<Item = PathBuf>
+    where
+        P: IntoIterator<Item = PathBuf>,
+    {
+        paths
     }
 
     #[cfg(windows)]
-    /// Check if given path with platform specification is valid
-    pub fn check_with_exe_extension<T: AsRef<Path>>(&self, path: T, binary_checker: &Checker) -> Option<PathBuf> {
+    fn append_extension<P>(paths: P) -> impl IntoIterator<Item = PathBuf>
+    where
+        P: IntoIterator<Item = PathBuf>,
+    {
         // Read PATHEXT env variable and split it into vector of String
-        let path_exts = env::var_os("PATHEXT").unwrap_or(OsString::from(env::consts::EXE_EXTENSION));
+        let path_exts =
+            env::var_os("PATHEXT").unwrap_or(OsString::from(env::consts::EXE_EXTENSION));
+
         let exe_extension_vec = env::split_paths(&path_exts)
             .filter_map(|e| e.to_str().map(|e| e.to_owned()))
             .collect::<Vec<_>>();
 
-        // Check if path already have executable extension
-        if has_executable_extension(&path, &exe_extension_vec) {
-            if binary_checker.is_valid(path.as_ref()) {
-                Some(path.as_ref().to_path_buf())
-            } else {
-                None
-            }
-        } else {
-            // Check paths appended with windows executable extensions
-            // e.g. path `c:/windows/bin` will expend to:
-            // c:/windows/bin.COM
-            // c:/windows/bin.EXE
-            // c:/windows/bin.CMD
-            // ...
-            exe_extension_vec.iter()
-                .map(|e| {
-                    // Append the extension.
-                    let mut s = path.as_ref().to_path_buf().into_os_string();
-                    s.push(e);
-                    PathBuf::from(s)
-                })
-                .skip_while(|p| !(binary_checker.is_valid(p)))
-                .next()
-        }
+        paths
+            .into_iter()
+            .flat_map(move |p| -> Box<dyn Iterator<Item = _>> {
+                // Check if path already have executable extension
+                if has_executable_extension(&p, &exe_extension_vec) {
+                    Box::new(iter::once(p))
+                } else {
+                    // Appended paths with windows executable extensions.
+                    // e.g. path `c:/windows/bin` will expend to:
+                    // c:/windows/bin.COM
+                    // c:/windows/bin.EXE
+                    // c:/windows/bin.CMD
+                    // ...
+                    let ps = exe_extension_vec.clone().into_iter().map(move |e| {
+                        // Append the extension.
+                        let mut p = p.clone().to_path_buf().into_os_string();
+                        p.push(e);
+
+                        PathBuf::from(p)
+                    });
+
+                    Box::new(ps)
+                }
+            })
     }
 }

src/helper.rs

@@ -2,12 +2,11 @@ use std::path::Path;
 
 /// Check if given path has extension which in the given vector.
 pub fn has_executable_extension<T: AsRef<Path>, S: AsRef<str>>(path: T, exts_vec: &Vec<S>) -> bool {
-    match path.as_ref()
-        .extension()
-        .and_then(|e| e.to_str())
-        .map(|e| exts_vec.iter().any(|ext| e.eq_ignore_ascii_case(&ext.as_ref()[1..])))
-    {
-        Some(true) => true,
+    let ext = path.as_ref().extension().and_then(|e| e.to_str());
+    match ext {
+        Some(ext) => exts_vec
+            .iter()
+            .any(|e| ext.eq_ignore_ascii_case(&e.as_ref()[1..])),
         _ => false,
     }
 }
@@ -20,28 +19,22 @@ mod test {
     #[test]
     fn test_extension_in_extension_vector() {
         // Case insensitive
-        assert!(
-            has_executable_extension(
-                PathBuf::from("foo.exe"),
-                &vec![".COM", ".EXE", ".CMD"]
-            )
-        );
+        assert!(has_executable_extension(
+            PathBuf::from("foo.exe"),
+            &vec![".COM", ".EXE", ".CMD"]
+        ));
 
-        assert!(
-            has_executable_extension(
-                PathBuf::from("foo.CMD"),
-                &vec![".COM", ".EXE", ".CMD"]
-            )
-        );
+        assert!(has_executable_extension(
+            PathBuf::from("foo.CMD"),
+            &vec![".COM", ".EXE", ".CMD"]
+        ));
     }
 
     #[test]
     fn test_extension_not_in_extension_vector() {
-        assert!(
-            !has_executable_extension(
-                PathBuf::from("foo.bar"),
-                &vec![".COM", ".EXE", ".CMD"]
-            )
-        );
+        assert!(!has_executable_extension(
+            PathBuf::from("foo.bar"),
+            &vec![".COM", ".EXE", ".CMD"]
+        ));
     }
 }

src/lib.rs

@@ -20,14 +20,14 @@ extern crate libc;
 extern crate tempdir;
 
 use failure::ResultExt;
-mod finder;
 mod checker;
+mod error;
+mod finder;
 #[cfg(windows)]
 mod helper;
-mod error;
 
-use std::path::{Path, PathBuf};
 use std::env;
+use std::path::{Path, PathBuf};
 
 // Remove the `AsciiExt` will make `which-rs` build failed in older versions of Rust.
 // Please Keep it here though we don't need it in the new Rust version(>=1.23).
@@ -36,11 +36,11 @@ use std::ascii::AsciiExt;
 
 use std::ffi::OsStr;
 
-use finder::Finder;
 use checker::CompositeChecker;
-use checker::ExistedChecker;
 use checker::ExecutableChecker;
+use checker::ExistedChecker;
 pub use error::*;
+use finder::Finder;
 
 /// Find a exectable binary's path by name.
 ///
@@ -72,7 +72,7 @@ pub fn which<T: AsRef<OsStr>>(binary_name: T) -> Result<PathBuf> {
 pub fn which_in<T, U, V>(binary_name: T, paths: Option<U>, cwd: V) -> Result<PathBuf>
 where
     T: AsRef<OsStr>,
-    U: AsRef<OsStr>,
+    U: AsRef<OsStr> + Clone,
     V: AsRef<Path>,
 {
     let binary_checker = CompositeChecker::new()
@@ -344,7 +344,6 @@ mod test {
         );
     }
 
-
     #[test]
     #[cfg(windows)]
     fn test_which_relative_extension_case() {