Open jlazaro2 opened 1 month ago
I can code review this.
I can code review this.
I can review this!
I can also review this
Hi! This is a great implementation of a basic find command. Well done! The PR meets the core functionality outlined in the description. I liked the use of flags like -name
and -type
, and the logic for handling pattern matching with *
and ?
is well-structured. A few suggestions:
I see that you are using tester.txt for testing, but adding more varied test cases (such as those with deeply nested directories or missing files) would enhance coverage.
While testing, I discovered:
find . -name non_existing_file.txt
, a message could be returned (e.g., No matching files found
).find . -testflag sample.txt
should print maybe a usage error or handle the unknown flag gracefully.In terms of code readability, you've done an great job with all the documentation and comments. The code was easy to follow. One suggestion: In the find()
function, the core logic for handling the flags (-type, -name) could be extracted into a separate helper function to improve readability and modularity.
High Level Checks:
Code Checks:
Overall, superb job! Thank you.
The PR complies with all the posted specifications for the find command. The code is nicely streamlined and there is not much requiring fixing.
Suggestions:
JavaDocs: add more information for the header comment in find.c
such as the description, flags, and how to run it.
Add more comments within functions to clarify what each block is doing such as the while loop in find()
.
In this if statement (line 80-84):
// if it's a directory, traverse it
if (st.type == T_DIR) {
if (strlen(path) + 1 + DIRSIZ + 1 > sizeof(buf)) {
printf("find: path too long\n");
return;
}
close fd
before returning.
Add more test cases such as more complex patterns to match for the name flag (e.g. a mix of ?
's and *
's).
Print an error message/usage when the syntax of the find
command is incorrect in anyway such as an invalid flag, wrong order of arguments or even too many arguments.
Add a help flag such as -h
which would print the usage information.
High Level Checks
[x] PR fully resolves the task/issue
[x] Does the PR have tests?
[x] Does the PR follow project code formatting standards?
[x] Does the OS still compile and run?
[x] Is documentation included?
Code Checks
[x] Does the code achieve its intended purpose?
[ ] Were all edge cases considered?
[x] Is the code organized and maintainable?
[x] Does the code maintain the same level of performance? (no performance regressions)
High Level Checks:
[✓] PR fully resolves the task/issue
[✓] Does the PR have tests?
[✓] Does the PR follow project code formatting standards?
[✓] Does the OS still compile and run?
[✓] Is documentation included?
Code Checks:
[✓] Does the code achieve its intended purpose?
[ ] Were all edge cases considered?
[✓] Is the code organized and maintainable?
[✓] Does the code maintain the same level of performance? (no performance regressions)
Hi! The code implementation achieves the suggested purpose specified in the PR outline. While having a good description of how to test the PR as well as the current limitations, the formatting is good and documentation is included. Great job!
A few suggestions: During testing I tried: find . -type [missing next argument] Result: returned find . -hello (improper flag) Result: returned mkdir /testdir mkdir /testdir/moreDir echo "test" > /testdir/moreDir/sample.md find /testdir/moreDir/ -name sample.md Result: /testdir/moreDir//sample.md
This may seem redundant but in agreement to the other reviewers I also agree that another approach to testing could have been to provide test directories and nested test directories into FogOS-find: testdir/example.txt testdir/moreexample.c testdir/evenmoree.md or testdir/cDir (subdirectory with C files) then testdir/txtDir (another subdirectory with txt files)
Overall, amazing work!
This is really nice, self contained, and perfect for P1. I don't have any reservations merging this because it's a great start.
There are some things that I ran into that were surprising coming from the usual find command:
find
by itself is usually okay, it matches any file and defaults to .
find <dir>
usually matches anything in the dir providedI like the support for checking file / dir types. As mentioned in the other CRs, I think the only major issue here is with how command lines are handled. As is, it's pretty brittle and expects things to be input in the exact order or it won't work, and there's no help info supplied (and nothing added to /docs to explain either). Instead of hard coding the argument positions, I'd handle them up front by searching for args that begin with -
and then set up a struct with your program options: what the pattern is, the dir, and whether to check for files/dirs. Then you'd pass those options in to find
and go from there -- no need to strcmp
against args to see what you need to do.
Overall this is minor, you can earn +0.25 for implementing improved arg parsing. Overall very well done.
Teammate: Esha Dupuguntla @eshadupz
Find command
Description The find command prints out the path of a file or directory based on the start of a directory given within the command. Syntax to search for a file is formatted like this: find <flag/option> <pattern/name of file or directory>
Additional flags
Changed files
Limitations
Example run in make qemu to test make qemu $ mkdir /testdir $ echo "test" > /testdir/sample.txt $ find /testdir -name sample.txt
other commands that can be run using find: $ find . -name tester.txt $ find . -name *.txt $ find /testdir -name sample.txt (double check that testdir exists) $ find . -type d $ find . -type f $ find /testdir -type f