Birmingham | 2026-MAR-SDC | Joy Opachavalit | Sprint 3 | Implement shell tools by enjoy15 · Pull Request #408 · CodeYourFuture/Module-Tools

enjoy15 · 2026-03-21T13:41:23Z

Learners, PR Template

Self checklist

I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
My changes meet the requirements of the task
I have tested my changes
My changes follow the style guide

Changelist

Completed all exercises

illicitonion

Generally looking good, but it looks like you're slightly over-fitting to the examples in the README.md - give some thought to other command lines a user may try to run, and how your implementations may be surprising to them.

illicitonion · 2026-03-27T17:04:46Z

+  let lineNumber = 1;
+
+  files.forEach((file) => {
+    const filePath = path.resolve(file);


What is this line doing? What would break if you removed it and just used file instead of filePath?

I have removed the unnecessary use of path.resolve and now directly use file. The code works as expected without it, simplifying the implementation.

illicitonion · 2026-03-27T17:06:21Z

+        if (options.numberNonEmpty && line.trim()) {
+          console.log(`${lineNumber}\t${line}`);
+          lineNumber++;
+        } else if (options.numberLines) {
+          console.log(`${lineNumber}\t${line}`);
+          lineNumber++;
+        } else {
+          console.log(line);


The three branches here look quite similar and repetitive. In general, if you have multiple similar branches, it's more clear to extract the differences into variables, and then run the same code, i.e. so you'd only have one call to console.log which looks more like console.log(`${prefix}${line}\n`) where prefix may be set differently based on options (including potentially an empty string).

This way it's easier for someone reading the code to see what's the same / different in each case, and also avoids the hazard that someone updates one of the branches but forgets to update the other ones.

I have refactored the code to reduce repetition. Now, the differences are handled using a prefix variable, and there is only one console.log statement. This makes the code cleaner and easier to maintain.

illicitonion · 2026-03-27T17:06:54Z

+        }
+      });
+    } catch (err) {
+      console.error(`cat: ${file}: No such file or directory`);


What exit code will your program have if something went wrong? What exit code should it have?

I have added proper exit codes. The program now exits with process.exit(1) when an error occurs, ensuring it signals failure appropriately.

illicitonion · 2026-03-27T17:07:55Z

+        }
+      });
+    } catch (err) {
+      console.error(`cat: ${file}: No such file or directory`);


Are you sure the problem here is that the file didn't exist? What would happen e.g. if you didn't have permission to read the file?

In general specific error messages are good, but misleadingly specific error messages are a problem - if we're not sure what went wrong (or if we have more information about what went wrong), we should present that information, rather than guessing. And if we are guessing, we should make it clear we're not sure what the problem exactly was and that this is a guess.

I have improved the error handling to differentiate between file-not-found (ENOENT) and permission errors (EACCES). The error messages now provide more accurate information about what went wrong.

illicitonion · 2026-03-27T17:16:07Z

+    if (arg === '-l') {
+      options.lines = true;
+    } else if (arg === '-w') {
+      options.words = true;
+    } else if (arg === '-c') {
+      options.bytes = true;
+    } else {
+      files.push(arg);
+    }


What happens if someone specifies more than one of these flags? What should happen?

Given the test cases we gave you in the README file, it's ok if your implementation doesn't do the same thing as the real wc does, though that would be ideal, but in general ignoring user input is bad - so if someone asks for both -l and -c and you ignore one of them, that can be confusing. Either showing both, or giving an error, is probably preferable.

I have updated the code to handle multiple flags. If multiple flags are specified, the program now displays all the requested results together, ensuring user input is respected.

This fixed some problems, but introduced some new ones too :)

What happens if you wc /some/file with no flags? What does your program do?

Also, what happens if you wc -l /some/file /some/other/file? What does your program do? What's different between them?

Thank you for pointing this out! Here's how the program currently behaves:

If no flags are provided (e.g., wc /some/file), the program defaults to displaying all metrics (lines, words, and bytes) for the file. This behavior is consistent with the standard wc command.

If multiple files are provided with a single flag (e.g., wc -l /some/file /some/other/file), the program outputs the line count for each file on separate lines. However, it does not provide a total count across all files, which I’ve made the updates to align with the standard wc behavior.

illicitonion · 2026-03-27T17:16:17Z

+  }
+
+  files.forEach((file) => {
+    const filePath = path.resolve(file);


Again, why do you need the path.resolve here?

I have removed the unnecessary use of path.resolve and now directly use the file path provided by the user. This simplifies the code without affecting functionality.

illicitonion · 2026-03-27T17:20:22Z

+    } else if (arg === '-a') {
+      options.all = true;
+    } else {
+      directories = [arg];


This meets the requirements for the examples listed in the README.md, but feels like it risks being confusing to users. If someone specified ls /some/path /some/other/path what do you think they would expect to happen? What does your code actually do? (See also the comment about wc options for a similar question)

I have updated the code to handle multiple directories. The program now lists files for all specified directories, making it more intuitive for users.

illicitonion

This is looking much better! Just a couple of things to look at still :)

illicitonion · 2026-04-07T09:17:49Z

+      const lines = data.split('\n');
+
+      lines.forEach((line) => {
+        const prefix = options.numberNonEmpty && line.trim() ? `${lineNumber}\t` : options.numberLines ? `${lineNumber}\t` : '';


Nested ternary operators are generally quite hard to read (and also, two of the conditions here lead to the same result, which is duplication) - I'd recommend rewriting this without a ternary.

Thank you for pointing this out! I have refactored the code to replace the nested ternary operator with a more readable if-else structure. This change eliminates the duplication and improves clarity by explicitly handling the conditions for setting the prefix.

illicitonion · 2026-04-07T09:23:24Z

+    if (arg === '-l') {
+      options.lines = true;
+    } else if (arg === '-w') {
+      options.words = true;
+    } else if (arg === '-c') {
+      options.bytes = true;
+    } else {
+      files.push(arg);
+    }


This fixed some problems, but introduced some new ones too :)

What happens if you wc /some/file with no flags? What does your program do?

Also, what happens if you wc -l /some/file /some/other/file? What does your program do? What's different between them?

…results

illicitonion · 2026-04-13T10:55:44Z

+        if (options.numberNonEmpty && line.trim()) {
+          prefix = `${lineNumber}\t`;
+        } else if (options.numberLines) {
+          prefix = `${lineNumber}\t`;
+        }
+        console.log(`${prefix}${line}`);
+        if (prefix) lineNumber++;


This works and is clear to read, but I may consider rewriting it as something like:

Suggested change

if (options.numberNonEmpty && line.trim()) {

prefix = `${lineNumber}\t`;

} else if (options.numberLines) {

prefix = `${lineNumber}\t`;

}

console.log(`${prefix}${line}`);

if (prefix) lineNumber++;

const shouldNumberLine = options.numberLines || (options.numberNonEmpty && line.trim());

if (shouldNumberLine) {

prefix = `${lineNumber}\t`;

lineNumber++;

}

console.log(`${prefix}${line}`);

Two reasons for this:

It shows that both of the "should number line" cases are handled the same.

The "should you increment the line count" is really based on "whether you should number the line", not "whether there's a prefix". I can imagine in the future we may add prefixes for different reasons not just line numbering, at which point if (prefix) lineNumber++; would now be incorrect. We generally want our conditions to be based on the underlying reason something should be done, not something else we decided to do for the same reason.

I’ve applied this refactor now. I introduced a shouldNumberLine condition and based both prefix assignment and lineNumber increment on that condition, so the logic is clearer and tied to the underlying behavior rather than checking prefix.

illicitonion · 2026-04-13T10:57:48Z

+    const bytes = Buffer.byteLength(data, 'utf8');
+
+    const results = [];
+    if (options.lines || (!options.lines && !options.words && !options.bytes)) results.push(lines);


This works but it doesn't read very well and is quite brittle - imagine if in the future you added a new option, you'd need to update all of these places where you're checking if no other options are specified.

Right now your options.lines perfectly mirrors the exact flags passed. But this doesn't have to be the case. Flags are just ways of configuring our program, but our program doesn't have to be tightly tied to the flags it exposes to users. Try making this line just be if (options.lines) { and see if you can initialise options.lines when parsing the flags to always have the correct answer for "should I show lines".

Thanks — updated now. I simplified flag handling as suggested, fixed -l counting logic, and aligned output formatting. Verified against all README wc commands.

enjoy15 added 3 commits March 21, 2026 12:22

complete cat excercise

1576d23

complete ls exercise

34fe1d0

complete wc exercise

daac5ec

enjoy15 added 📅 Sprint 3 Assigned during Sprint 3 of this module Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Mar 21, 2026

illicitonion reviewed Mar 27, 2026

View reviewed changes

illicitonion added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Mar 27, 2026

Refactor file handling and error messages in cat, ls, and wc tools

7e70d1c

enjoy15 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 5, 2026

illicitonion reviewed Apr 7, 2026

View reviewed changes

illicitonion removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 7, 2026

Enhance cat and wc tools: improve line numbering logic and aggregate …

a3dc8ae

…results

enjoy15 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 10, 2026

illicitonion reviewed Apr 13, 2026

View reviewed changes

illicitonion removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 13, 2026

enjoy15 added 2 commits April 15, 2026 22:57

Refactor cat and wc tools: improve line handling and output formatting

9f4f7aa

Refactor cat tool: simplify line handling and improve output formatting

054db41

enjoy15 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 15, 2026

Uh oh!

Conversation

enjoy15 commented Mar 21, 2026

Learners, PR Template

Changelist

Uh oh!

illicitonion left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

illicitonion left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants