Skip to content

Mirror of 437 2#11

Open
illicitonion wants to merge 4 commits intomainfrom
mirror-of-437-2
Open

Mirror of 437 2#11
illicitonion wants to merge 4 commits intomainfrom
mirror-of-437-2

Conversation

@illicitonion
Copy link
Copy Markdown
Member

No description provided.

@illicitonion illicitonion added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. NotCoursework labels Apr 10, 2026
@cyf-ai-code-reviewer
Copy link
Copy Markdown

Thanks for opening a new PR! AI started to review it

Copy link
Copy Markdown

@cyf-ai-code-reviewer cyf-ai-code-reviewer bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are many code comment that doesn't provide much value. Could you please check if some comments can be removed, for example comments that just repeat what code does?

// filter flags, and getting the string of filename
const filePaths = argv.filter((arg) => !arg.startsWith("-"));

const noFlags = !showLines && !showCharacters && !showWords && !showCharacters;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable 'noFlags' is intended to check if no flags are set, but the logic includes '!showCharacters' twice and omits '!showBytes'. This could be confusing to someone reading the code. How could you make the naming and logic clearer so that it accurately reflects the intent and is easy to understand?

Comment on lines +32 to +37
let output = "";

if (showLines || noFlags) output += `${lines} `;
if (showWords || noFlags) output += `${words} `;
if (showBytes || noFlags) output += `${bytes} `;
if (showCharacters || noFlags) output += `${characters} `;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You assign the result of fs.readFileSync(filePath, "utf-8") to 'content', and then use it immediately for processing. Since you use 'content' multiple times, this seems reasonable. However, for the 'output' variable, you build up a string by appending to it in several if statements. Do you think there is a clearer way to construct the output string, perhaps by collecting the values in an array and joining them, to avoid the need for a mutable variable?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Droid-An This comment seems quite odd?

The "content"-related stuff feels irrelevant. And I'm not sure accumulating in an array is any cleaner than mutating a string? There may be other benefits (e.g. avoiding O(n^2) copies repeatedly appending), but avoiding a mutable variable by using something that happens to be internally mutable because it's a reference type seems like an odd suggestion?

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

Labels

Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. NotCoursework

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants