Repository navigation
Implement ls and wc command line tools in Python - #18
matthewjustice wants to merge 1 commit into
Conversation
| entries = sorted(Path(args.directory).iterdir(), key=lambda entry: entry.name) | ||
| for entry in entries: | ||
| if args.all or not entry.name.startswith("."): | ||
| print(entry.name) |
There was a problem hiding this comment.
In main, the entries variable is created only to be used immediately in the for loop:
- Line 16:
entries = sorted(Path(args.directory).iterdir(), key=lambda entry: entry.name) - Line 17:
for entry in entries:
Because entries isn’t reused elsewhere, one question to consider is whether introducing this temporary name actually helps readability for you, or whether you’d find it just as clear to loop directly over the sorted iterator. When a variable is introduced and only used once right away, it can sometimes add a tiny bit of cognitive overhead for readers who then need to remember that entries is simply the sorted directory listing.
How do you feel about the clarity here? If you find that the name entries meaningfully improves understanding, it can be worth keeping. If not, you might explore iterating directly over the sorted result to reduce the number of moving parts in the function.
To "like" or "dislike" this comment, please follow this link
| def print_counts(counts, label): | ||
| values = [str(value) for value, enabled in zip(counts, selected) if enabled] | ||
| print(" ".join([*values, label])) |
There was a problem hiding this comment.
Inside print_counts, the values list is created and then immediately used once in the print call:
- Line 25:
values = [str(value) for value, enabled in zip(counts, selected) if enabled] - Line 26:
print(" ".join([*values, label]))
Since values isn’t reused, you might ask yourself whether naming this intermediate result makes the code easier for you (and others) to follow, or whether it could be inlined into the print call without losing clarity. Temporary variables that are only used once can sometimes obscure the main intent of a small helper like this, though they can also make complex expressions easier to read.
Would an inlined version still feel readable to you, or does having values as a named step make the logic clearer in your eyes? Thinking about that trade-off can help you decide when a one‑use temporary is pulling its weight.
To "like" or "dislike" this comment, please follow this link
| @@ -0,0 +1,47 @@ | |||
| """Count lines, words, and bytes in files.""" | |||
There was a problem hiding this comment.
In wc.py, all the naming and text around the counts suggest that the third value is a byte count, but the implementation actually computes a character count, which can differ from the byte count for UTF-8 text containing non-ASCII characters.
Concretely:
- The module docstring says: "Count lines, words, and bytes in files." (line 1)
- The
-cflag’s help says: "print byte counts" (line 17). count_filereads the file as bytes (line 9), decodes it as UTF-8, and then returnslen(text)as the third value (lines 9–11). After decoding,len(text)is the number of Unicode codepoints in the string, not the number of bytes in the original file.
So as a reader (or user of this script), you’d believe -c and the third count represent raw bytes, but for multi-byte UTF-8 characters the number you get will actually be smaller than the real byte size. That mismatch between the names/descriptions and the actual value is what makes this confusing.
You might find it useful to ask yourself: if someone else tried to reuse count_file because they needed byte counts, what assumptions would they reasonably make based on the function name, the docstring, and the option help text? How could you adjust either the naming/documentation or the implementation so that the third count and the "byte" wording are in clear agreement?
To "like" or "dislike" this comment, please follow this link
|
The CYF AI review has left comments. It will only review a PR one time. When you have addressed these comments, please request review again and a volunteer will take a look. |
This PR is for testing the cyf-ai-code-reviewer