London | 26-SDC-Mar | Mariia Serhiienko | Sprint 3 | Implement shell tools by Konvaly · Pull Request #499 · CodeYourFuture/Module-Tools

Konvaly · 2026-04-17T01:14:26Z

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

Implemented three shell tools in NodeJS:

cat - prints file contents, supports -n (number all lines) and -b (number non-blank lines only)
ls - lists directory contents, supports -1 (one per line) and -a (show hidden files)
wc - counts lines, words and bytes in files, supports -l, -w, -c flags and multiple files with a total row

Each tool matches the output of the real Unix commands.

illicitonion

Generally looking good, but some bits of duplication to think about :)

illicitonion · 2026-04-21T12:35:30Z

+if (args.length === 0) {
+  console.error("Usage: node cat.js [-n] [-b] <file...>");
+  process.exit(1);
+}
+const showAllLineNumbers = args.includes("-n");
+const showNonBlankNumbers = args.includes("-b");


Imagine if I called this programme with a different flag, e.g. node cat.js -x /some/file. I probably passed that -x flag expecting it would do something. But your programme just ignores it.

What do you think we should do if we run into a flag our programme doesn't understand?

This applies to all three tools.

Thanks for pointing this out! You're right, silently ignoring unknown flags is confusing for the user. I've added a check in all three tools that shows an error message and exits if the user passes a flag we don't recognise. For example, node cat.js -x file.txt will now print cat: invalid option -- 'x' instead of just running as if nothing happened.

illicitonion · 2026-04-21T12:41:05Z

+for (const filePath of filePaths) {
+  const content = await fs.readFile(filePath, "utf-8");
+
+  if (!showAllLineNumbers && !showNonBlankNumbers) {


These are really clear variable names, which makes this if statement read very clearly - well done!

illicitonion · 2026-04-21T12:42:42Z

+      let isLastLine;
+      if (i === lines.length - 1) {
+        isLastLine = true;
+      } else {
+        isLastLine = false;
+      }


This could be a bit more concise - generally every time you have code of the form:

if (condition) { true } else { false }

you can probably use the fact that condition is a boolean, to just use that expression. e.g.

const isLastLine = i === lines.length - 1;

Ah yes, that makes total sense! I've simplified it. Much cleaner, thanks for the tip!

illicitonion · 2026-04-21T12:44:15Z

+        break;
+      }
+
+      if (showAllLineNumbers) {


The code in these two branches is quite repetitive - both do padding, hard-code what number we're padding, etc.

Can you think how to write this in such a way that we don't need to repeat e.g. the padding twice?

You're right, I've refactored it. Now I first figure out whether the line needs a number using const needsLineNumber = showAllLineNumbers || (showNonBlankNumbers && !isBlankLine), and then do the padding only once inside a single if block. This way if I ever need to change the formatting, I only change it in one place.

illicitonion · 2026-04-21T12:58:35Z

+
+const paths = args.filter((arg) => !arg.startsWith("-"));
+
+const targetPath = paths.length > 0 ? paths[0] : process.cwd();


If I call this programme with more than one argument (e.g. ls /tmp /dev), it will ignore the second one and just list the first. As a user, what would you expect to happen here?

I'd definitely expect it to show both directories. I've fixed it , now it loops through all the paths, not just the first one. It also shows directory names as headers when there's more than one, like the real ls does.

illicitonion · 2026-04-21T14:10:02Z

+  const cleanA = a.replace(/^\.+/, "");
+  const cleanB = b.replace(/^\.+/, "");
+  return cleanA.localeCompare(cleanB);


What's this "cleaning" doing? What would break if you removed it?

This removes leading dots from filenames before comparing them in the sort. Without it .hidden.txt would appear before 1.txt because . comes before numbers in ASCII.

illicitonion · 2026-04-21T14:12:11Z

+  if (noSpecificFlag) {
+    process.stdout.write(formatLine([lines, words, bytes], filePath) + "\n");
+  } else if (showLines) {
+    process.stdout.write(formatLine([lines], filePath) + "\n");
+  } else if (showWords) {
+    process.stdout.write(formatLine([words], filePath) + "\n");
+  } else if (showBytes) {
+    process.stdout.write(formatLine([bytes], filePath) + "\n");
+  }


This is very repetitive - can you think how to make this less repetitive?

Agreed. I pulled the logic into a getCounts() function that builds the right array depending on which flags are set. So now instead of four near-identical branches, it's just two lines.

illicitonion · 2026-04-21T14:12:53Z

+  } else if (showLines) {
+    process.stdout.write(formatLine([lines], filePath) + "\n");
+  } else if (showWords) {


What happens with the real wc if I pass multiple flags (e.g. wc -wc /some/file)? What does your implementation do?

Real wc -wc shows both words and bytes. Mine didn't because the else if meant only one could win. Fixed it, combined flags like -wc and -lw work now.

… multiple directories in ls

illicitonion

This is looking great, well done!

A couple of small things to pick up, and then a few things to think about that don't block marking this as complete :)

illicitonion · 2026-04-29T09:44:28Z

+);
+
+if (unknownFlags.length > 0) {
+  console.error(`cat: invalid option -- '${unknownFlags[0].slice(1)}'`);


Optional: It's often useful to tell people about all of the problems you find, if it's easy to do so, so they can fix all of them - the experience of running:

% cat -f -x /tmp/t cat: invalid option -- f % cat -x /tmp/t cat: invalid option: -- x % cat /tmp/t It worked!

is quite frustrating compared to:

% cat -f -x /tmp/t cat: invalid options: f, x % cat /tmp/t It worked!

illicitonion · 2026-04-29T09:48:28Z

+  try {
+    entries = await fs.readdir(targetPath);
+  } catch (error) {
+    console.error(


Currently if there's an error (e.g. if you specify a path that doesn't exist), the exit code of this program will be 0. Is that what we want?

illicitonion · 2026-04-29T09:50:20Z

+const expandedArgs = [];
+for (const arg of args) {
+  if (arg.startsWith("-") && arg.length > 2) {
+    for (const char of arg.slice(1)) {
+      expandedArgs.push(`-${char}`);
+    }
+  } else {
+    expandedArgs.push(arg);
+  }
+}
+
+const showLines = expandedArgs.includes("-l");
+const showWords = expandedArgs.includes("-w");
+const showBytes = expandedArgs.includes("-c");
+
+const supportedFlags = ["-l", "-w", "-c"];
+const unknownFlags = expandedArgs.filter(
+  (arg) => arg.startsWith("-") && !supportedFlags.includes(arg),
+);
+
+if (unknownFlags.length > 0) {
+  console.error(`wc: invalid option -- '${unknownFlags[0].slice(1)}'`);
+  process.exit(1);
+}


This is good code, and works well considering different edge cases (well done!), but is a lot of code you've had to write to handle this :) you don't need to for this exercise, but you may want to look at a library like https://github.com/tj/commander.js which can do a lot of this for you.

illicitonion · 2026-04-29T09:52:22Z

+  if (noSpecificFlag || showLines) counts.push(lines);
+  if (noSpecificFlag || showWords) counts.push(words);
+  if (noSpecificFlag || showBytes) counts.push(bytes);


These work, but quite tightly couple the CLI flags to the behaviour here. We don't need to use only what flags we happened to expose to users when writing our logic - we can transform things ourselves. Can you think how you could set up showLines, showWords and showBytes when parsing the flags so that these checks would just read:

if (showLines) counts.push(lines); if (showWords) counts.push(words); if (showBytes) counts.push(bytes);

but still work correctly when not flags were passed?

Konvaly added 3 commits April 17, 2026 01:13

implement cat with -n and -b flags

b40010c

implement ls with -1 and -a flags

19cae55

implement wc with -l -w -c flags

ccf9aa6

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

illicitonion added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Apr 21, 2026

illicitonion reviewed Apr 21, 2026

View reviewed changes

illicitonion added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Apr 21, 2026

Konvaly added 2 commits April 27, 2026 23:45

address PR feedback: unknown flags, combined flags, reduce repetition

f3923c8

address PR feedback: unknown flags check, simplify cat logic, support…

5c84e31

… multiple directories in ls

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

illicitonion reviewed Apr 29, 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 Apr 29, 2026


		const paths = args.filter((arg) => !arg.startsWith("-"));

		const targetPath = paths.length > 0 ? paths[0] : process.cwd();

Uh oh!

Conversation

Konvaly commented Apr 17, 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!

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