Clone of 390#14
Conversation
illicitonion
added
NotCoursework
Needs Review
|
Thanks for opening a new PR! AI started to review it. Please notice that AI will review this PR only once |
![cyf-ai-code-reviewer[bot]](https://avatars.githubusercontent.com/u/22743767?s=60&v=4){kind=small}
| stats = os.stat(file_path) | ||
| count = {"lines": 0, "words": 0, "bytes": stats.st_size} |
There was a problem hiding this comment.
In main, the stats variable (line 44) is created only to immediately use stats.st_size when building the count dictionary on the next line. Because stats isn’t reused elsewhere, this temporary variable doesn’t add much clarity and slightly increases the mental overhead when reading the loop.
You might ask yourself: would it be clearer to access the file size directly where it’s needed, or does having a named stats object help you understand the code better? In this particular case, since only the size is used, inlining the os.stat(...).st_size expression into the dictionary could make the code a bit more direct without sacrificing readability.
Thinking along these lines can help you decide when a temporary variable is pulling its weight versus when it’s just an extra name to track.
To "like" or "dislike" this comment, please follow this link
| output_columns = [ | ||
| "lines", "words", "bytes"] if no_options_provided else selected_option_keys | ||
| rows: list[list[str | int]] = [] | ||
| for name, values in results.items(): | ||
| rows.append([name] + [values[column] for column in output_columns]) | ||
|
|
||
| if no_options_provided: | ||
| print(tabulate(rows, headers=[ | ||
| "index"] + output_columns)) | ||
| else: | ||
| print(tabulate(rows, headers=[ | ||
| "index"] + output_columns)) |
There was a problem hiding this comment.
In the tabulated output, the first column header is set to "index", but the values you put in that column are actually the file path (or the string "total"), not a numeric index. This can easily mislead someone reading the code or the output into thinking that column contains row numbers.
You might find it useful to ask yourself: if I saw a table with a column called index, what would I expect to be inside it? How could you rename the header so that it more accurately reflects that it holds the file name/path (and total), and keeps the code easier to understand for future readers?
This affects both branches where you call tabulate, since both use the same header list.
To "like" or "dislike" this comment, please follow this link
| st = os.stat(file_path) | ||
| # Is a file? | ||
| if stat.S_ISREG(st.st_mode): | ||
| result_files.append(file_path) | ||
| # Is a directory? | ||
| if stat.S_ISDIR(st.st_mode): | ||
| result_dirs[file_path] = os.listdir(file_path) |
There was a problem hiding this comment.
On these two lines you added comments # Is a file? and # Is a directory?. In this case, the code right below (stat.S_ISREG and stat.S_ISDIR) is already quite clear about what it’s doing, so the comments don’t add much information. Over time, comments that merely restate the code can become noise and make it harder to spot the comments that do carry important context (like explaining a tricky edge case or a non-obvious design decision).
How might you decide when a comment is actually clarifying something non-obvious versus just repeating what the code already says? In this particular case, could choosing slightly more descriptive variable or helper names (for example, extracting a small function that classifies paths) remove the need for these comments altogether?
To "like" or "dislike" this comment, please follow this link
illicitonion
added
Needs Review
2 participants
No description provided.