Skip to content

Conversation

@pjpatil30
Copy link

General Overview: count command is similar to grep, as it looks through files for a specific word, and lists the number of times the word appeared within each file, and provides a total count at the end.

@likhithabz55
Copy link

I'll be reviewing this PR

@likhithabz55
Copy link

likhithabz55 commented Oct 4, 2025

Verification :

Issue :
This command would essentially be similar to grep, as it would look through the files for a specific word, function call, phrase, etc, and then add up the counts within each file in the specified directory. It would display the file name first, followed by the keyword searched for, and lastly the count. One note is that it will display each count by the files, but will also implement a total count at the end that displays how many total times that keyword/s appeared within the directory.

The implementation covers all the requirements mentioned in the issue. The command "count " is working as expected.

Testing :

I have tried running the tests mentioned in the documentation, along with few of my own. Most of the test cases are passing.

I noticed just one edge case which is not working as expected though. I mentioned it below.

Code Walkthrough :

Great job!! The code looks clean and well formatted. I would just suggest looking into one thing.

Right now, if a file contains longer lines (> 512 bytes), the count word is returning 0. I would suggest handling this either by printing a message to convey the issue or modifying the logic inside count_in_file method to be able to dynamically handle lines with arbitrary lengths.

Screenshot 2025-10-03 at 7 10 29 PM

Documentation is present

Formatting

The formatting is consistent and the indentation is clean. I would just suggest adding comments above the methods and removing comments inside some methods in count.c so that it would make it easier to understand .

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?

The implementation is solid. It handles most of the edge cases. i would just suggest looking into how to handle larger lines.

@aryianj
Copy link

aryianj commented Oct 4, 2025

I'll be reviewing this

@aryianj
Copy link

aryianj commented Oct 4, 2025

This command works exactly as expected. I would suggest adding more detail to your count.md file. Currently, for your command options, it just says count or count ..., but you could change it to something like "count [word] [directory]" so it's clearer to understand how to run the code. Additionally, there are some indentation errors in your code. Mostly in count and count_in_file. Otherwise, you did a great job and fulfilled everything you set out to do!

@PiePaing
Copy link

PiePaing commented Oct 6, 2025

I'll be code reviewing this

@stevenbuks
Copy link

Overall the code looks good and I can see your test cases in your readme. All the main stuff is present but I am confused about what the KMP algo is as there was no explanation for that. I do like how if I enter an input that isn't valid there is some error handling. I would maybe just want the code to output only the files where there is a match and not all the files in total. Other than that I like this

High Level Checks:

  • [X ] Is documentation included? This should explain how to build, run, and test the software.
  • [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?

Code Checks:

  • [ X] Does the code achieve its intended purpose?
  • [ X] 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)

@malensek
Copy link
Contributor

Cool idea, and the implementation looks good! Thanks for already addressing some of the previous CR comments!

I am a bit iffy on the listed of visited inodes... I'd exclude '.' and '..' instead because now we can only fit 22 entries in the list and that seems too small for even a toy OS like we are using. Maybe you were building that because of worries about links? I can't find the justification, maybe I missed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants