I too will assume you want zero deps. But instead of presenting a packaged result, I'll walk through making some changes, to illustrate the process of spotting and making iterative improvements. I mostly try to preserve current behavior -- so the result isn't necessarily the "best solution" so much as just cleaner code (IMO).
If we look at the constructor and iterator implementation, we can see that either
- The constructor sets
parent and dir to Some
- In which case the iterator never sets either to
None
- Or the constructor sets
parent to Some and dir to None
- In which case the iterator returns one item
So an easy first change in next is to get rid of the if-else chain and early return if dir is None, getting rid of one nesting level.
fn next(&mut self) -> Option<Self::Item> {
let Some(dir) = &mut self.dir else {
return self.parent.take().map(|p| p.display().to_string());
};
let pattern_len = self.before.len() + self.after.len();
// ...
Next, this is basically a for loop with a filter:
loop {
match dir.next() {
None => break None,
Some(entry) => {
if let Ok(entry) = entry { ... } // no `else`
So we can use for to remove multiple levels of nesting.
for entry in dir.filter_map(|e| e.ok()) {
let file_name = entry.file_name().display().to_string();
// ... no `continue`, `break` becomes `return`, `None` after `for`
And get rid of one more if-else by utilizing the logical invariant that parent is Some if we can reach the for loop.
let Some(parent) = &self.parent else { unreachable!() };
for entry in dir.filter_map(|e| e.ok()) {
We've only really looked at flow and I think this is already a significant improvement.
Now thinking about the logic that's left in next, this is basically another map (we only use entry.file_name()) and filter. It's easy to only call file_name() once, which is an improvement. But (given the restrictions of the constructor) we can also avoid converting to String to do the check: the only way the file name can start with begin is if its encoded bytes starts with the bytes of begin, and similarly for end.
Let's make this a standalone function as well.
fn matches(before: &str, after: &str, file_name: &OsStr) -> bool {
let bytes = file_name.as_encoded_bytes();
// You had `>` but I think that was just a bug
bytes.len() >= before.len() + after.len()
&& bytes.starts_with(before.as_bytes())
&& bytes.ends_with(after.as_bytes())
}
At this point, I think we might as well return PathBuf too, as we no longer need to convert to String or str for the comparison.
impl Iterator for Glob {
type Item = PathBuf;
fn next(&mut self) -> Option<Self::Item> {
let Some(dir) = &mut self.dir else {
return self.parent.take();
};
let Some(parent) = &self.parent else { unreachable!() };
dir.filter_map(|e| e.map(|e| e.file_name()).ok())
.filter(|file| matches(&self.before, &self.after, &file))
.map(|file_name| parent.join(file_name))
.next()
}
}
We haven't try to improve the constructor yet, so let's do that now. For starters, we know all the components are valid UTF-8 as our input was a &str, and we can work with &Path instead of PathBuf and String in many cases.
let parent: &Path = str.as_ref();
if let Some(file_name) = parent.file_name()
&& let file_name = file_name.to_str().unwrap()
&& let Some((before, after)) = file_name.split_once('*')
{
let parent = parent.parent().unwrap_or(parent);
I know I said I would try to preserve behavior, but if we think about how you call read_dir...
Sidebar about unwrap_or_default and join
let dir = if parent.has_root() {
parent.read_dir()
} else {
current_dir().unwrap_or_default().join(parent).read_dir()
}
.unwrap();
If current_dir() fails, IMO you should also just fail -- something's probably pretty wrong for that to happen (and trying to read the current directory will probably fail too). There is also a subtle inconsistency here: if parent is empty, pushing it onto the default will leave you empty (""). But probably you wanted to fall back to ".". Or you could just use "." to begin with (which is load bearing if parent is empty).
So I think these are better.
let dir = if parent.has_root() {
parent.read_dir()
} else {
current_dir().unwrap().join(parent).read_dir()
}
.unwrap();
let dir = if parent.has_root() {
parent.read_dir()
} else {
Path::new(".").join(parent).read_dir()
}
.unwrap();
let dir = match parent.as_os_str().is_empty() {
true => Path::new(".").read_dir(),
false => parent.read_dir(),
}.unwrap();
Also while we're here, we can clean up a little more by making our two use cases explicitly separated (so we don't need unreachable!() in next for example).
struct GlobDir {
dir: ReadDir,
parent: PathBuf,
before: String,
after: String,
}
impl Iterator for GlobDir { ... }
enum GlobInner {
Parent(Option<PathBuf>),
GlobDir(GlobDir),
}
pub struct Glob {
glob: GlobInner,
}
impl Iterator for Glob {
type Item = PathBuf;
fn next(&mut self) -> Option<Self::Item> {
match &mut self.glob {
GlobInner::Parent(parent) => parent.take(),
GlobInner::GlobDir(gd) => gd.next(),
}
}
}
That's another good stopping point. Finally, note how we call to_owned() on everything but dir in the constructors in from. All those borrows came from the input &str. So if you don't mind the input &str remaining borrowed by the returned iterator, you can also get rid of those allocations. (With more boilerplate you could support both use cases.)
In the collapsed section, I talked about some of the subtleties around working with the std Path[Buf] types/methods. There's a lot more that could be said about this, and corner cases when working with parent, pop, file_name, and friends -- what they do does not always line up one-to-one. For instance a push followed by a pop can result in a different path (semantically, not just textually).
Not wanting to get into those weeds was part of why I (mostly) ignored potentially unwanted behavior.
One odd case you have that could be easily handled is an empty input &str.
Another alternative is to make from fallible (return Result).