Skip to content

Conversation

@ajsdkty3
Copy link

Description: This pull request enhances the wc command by adding new features to count punctuation, digits, and uppercase/lowercase letters in a file.

Features:

Character Type Counting: Alongside the standard counts (lines, words, characters), this enhancement introduces the ability to count:
Punctuation marks (e.g., ., ,, !, ?).
Digits (0-9).
Uppercase letters (A-Z).
Lowercase letters (a-z).

Options:
-p: Count punctuation marks.
-d: Count digits.
-u: Count uppercase letters.
-l: Count lowercase letters.
Testing: test_count.c is added to test wc on count_sample.txt

@geoweb999
Copy link

geoweb999 commented Sep 24, 2024

I'd be happy to code review

@geoweb999
Copy link

The original issue opened for enhancing wc proposes to enhance wc to include flags:
-l: Count lines.
-w: Count words.
-c: Count characters.
-p: Count punctuation marks.
-d: Count digits.
-u: Count uppercase letters.
-L: Count lowercase letters.

This implementation is missing -c, -L and -w flags. In addition, the -l flag now counts lower-case letters and not lines where lines is a very common use case so losing that functionality is an issue. I would also suggest adding a -h flag to print some simple help explaining what each flag does:

$ wc -h
wc -- print word count 
usage: wc [-l] [-w] [-c] [-p] [-d] [-u] [-L] [-h]
-l: Count lines.
-w: Count words.
-c: Count characters.
-p: Count punctuation marks.
-d: Count digits.
-u: Count uppercase letters.
-L: Count lowercase letters.
-h: print this helpful information

Adding a test-count program is a really nice touch. It might be helpful if the test program indicated how many tests passed and how many failed but otherwise it's great.

I did some manually testing and there's an edge case if you try to run wc on a directory:

$ mkdir foo
$ wc foo
0 1 32 foo
$ wc -p foo
Punctuation: 3

However, this is how wc works originally so I don't know if it's important to fix. Calling fstat to determine file type (T_DIR) would allow you to exclude directories.

A second issue is that pipes no longer work. Here's how the original FogOS looks:

$ cat README.md | wc
4 3 35 

And with the current version of wc:

$ cat README.md | wc
$ 

I think this line might be an issue: if (!flags_specified && name[0] != '\0') {

The code looks great, it's easy to understand what you are doing and the javadoc style comment is really excellent. I really liked how you cleaned up the bad formatting in the old version of wc, it's much easier to read now.

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)

@riyamathur1
Copy link

I can code review this

@riyamathur1
Copy link

Code review: Enhance wc command
This project is well implemented, easy to understand and test, and achieves the intended purpose.

Flags:
-p (Count Punctuation): Correctly counts punctuation marks, including periods, symbols, commas, exclamation points, etc.
-d (Count Digits): The digit count is accurate and handles all numeric characters from 0-9
-u (Count Uppercase Letters): Accurately counts all uppercase letters
-l (Count Lowercase Letters): Accurately counts all lowercase letters

Code Style:
The code is clean, consistent, and easy to understand. The javadoc comments for the wc function were helpful.

Edge Cases:
Multiple flags can be used simultaneously.
Good error handling for invalid flags.
Correct output for cases with no punctuation/digits/uppercase and lowercase letters.

Testing:
The test file test_count.c was very helpful and included a variety of test cases. count_sample.txt included punctuation, digits, and uppercase/lowercase letters.

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)

@malensek
Copy link
Contributor

It looks like the main functionality got added to this, but several regressions have happened as well. I immediately noticed I could no longer pipe data into wc, -l now means something different, there is no help message, etc. There is certainly a disconnect between the features outlined in the issue and this PR. However, including a test program is great!

If you fix the pipe issue, directory check, add help message, and update the flags to reflect the features you implemented you can earn +1 to your project.

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.

4 participants