London | 26-SDC-Mar | Ebrahim Beiati-Asl | Sprint 3 | implements-shell-tool by ebrahimbeiati · Pull Request #376 · CodeYourFuture/Module-Tools

ebrahimbeiati · 2026-03-14T14:44:41Z

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

Exercise for implement-shell-tools

illicitonion

This generally looks good, but I left a few comments to consider. Well done!

illicitonion · 2026-03-16T10:24:13Z

implement-shell-tools/cat/cat.js

+        if(arg === '-n') {
+            options.numberAll = true;
+        } else if(arg === '-b') {
+            options.numberNonEmpty = true;
+        } else {
+            filePatterns.push(arg);
+        }
+    });
+    // -b takes precedence over -n
+    if(options.numberNonEmpty) {
+        options.numberAll = false;
+    }


Rather than having two different options which are mutually exclusive, I would probably model this as one option with three possible values: options.numberMode = "off" | "all" | "non-empty". That way you don't need to re-set numberAll if actually numberNonEmpty - you just have one variable you set correctly up-front and which you can check.

illicitonion · 2026-03-16T10:24:35Z

implement-shell-tools/cat/cat.js

+        process.exit(1);
+    }
+
+    const files = filePatterns;


Why did you call this filePatterns just to rename it to files later? Why not just name it files or fileNames from the start?

illicitonion · 2026-03-16T10:25:02Z

implement-shell-tools/cat/cat.js

+    if(filePatterns.length === 0) {
+        console.log("cat: missing file operand");
+        process.exit(1);
+    }
+
+    const files = filePatterns;
+
+    files.forEach((file) => {
+        const resolvedPath = path.resolve(process.cwd(), file);


What advantage does path.resolve(process.cwd(), file) have over just using file directly?

illicitonion · 2026-03-16T10:26:02Z

implement-shell-tools/ls/ls.js

+            const names = filtered.map(entry => entry.name);
+            console.log(names.join('  '));
+        }
+        } catch (error) {


This indentation isn't correct - please format all of your code before submitting for review. I'd recommend setting up your IDE to auto-format your code on save to avoid needing to remember to do this.

illicitonion · 2026-03-16T10:30:28Z

implement-shell-tools/wc/wc.js

+    if(options.word) parts.push(counts.words);
+    if(options.byte) parts.push(counts.bytes);
+//if no specific count options are provided, print all counts
+    if(!options.line && !options.word && !options.byte) {


It may be simpler to do this check when setting up options, rather than when consuming it - i.e. in your options parsing to see "if no options were set, set all to true" - that way the rest of your code doesn't need to think about this special case, they can just handle wc foo and wc -l -w -c foo exactly the same.

(This about this as an abstraction boundary - the point of parsing command line flags into an options object is so that the rest of your code doesn't need to worry about the specifics of flags, this is just one more example of a place you can avoid your code needing to know the specifics of flags)

illicitonion · 2026-03-16T10:36:06Z

implement-shell-tools/ls/ls.js

+    const directories = args.filter(arg => arg !== '-a' && arg !== '-1');
+
+    // If no directory is specified, list the current directory
+    if(directories.length === 0) {


Can you think how to fold the "no directories" case into the main loop so you don't have to have two separate calls to listDirectory in this function?

illicitonion · 2026-03-16T10:38:51Z

implement-shell-tools/cat/cat.js

+        lines.forEach((line) => {
+            if(options.numberNonEmpty) {
+                //-b option: number non-empty lines
+                if(line.trim()) {
+                    process.stdout.write(
+                        `${String(globalLineCounter).padStart(6)}\t${line}\n`
+                    );
+                    globalLineCounter++;
+                } else {
+                    process.stdout.write('\n');
+                }
+            } else if(options.numberAll) {
+                //-n option: number all lines
+                process.stdout.write(
+                    `${String(globalLineCounter).padStart(6)}\t${line}\n`
+                );
+                globalLineCounter++;
+            } else {
+                //default: just print the line
+                process.stdout.write(line + '\n');
+            }
+    });


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 process.stdout.write which looks more like process.stdout.write(`${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.

illicitonion

Looking really good, great improvements! A few more small things to think about :)

illicitonion · 2026-03-16T13:00:30Z

implement-shell-tools/cat/cat.js

+      let prefix = "";
+
+      if (options.numberMode === "non-empty") {
+        //-b option: number non-empty lines


This comment feels redundant - reading the if condition tells me the same information as the comment does

illicitonion · 2026-03-16T13:01:47Z

implement-shell-tools/cat/cat.js

+      if (options.numberMode === "non-empty") {
+        //-b option: number non-empty lines
+        if (line.trim() !== "") {
+          prefix = `${String(globalLineCounter).padStart(6)}\t`;
+          globalLineCounter++;
+        }
+      } else if (options.numberMode === "all") {
+        prefix = `${String(globalLineCounter).padStart(6)}\t`;
+        globalLineCounter++;


There's still some repetition here in setting up the prefix. How about something like:

if (options.numberMode === "all" || (options.numberMode === "non-empty" && line.trim() !== "") {

illicitonion · 2026-03-16T13:03:25Z

implement-shell-tools/ls/ls.js

+      filtered.forEach((entry) => console.log(entry.name));
+    } else {
+      const names = filtered.map((entry) => entry.name);
+      console.log(names.join("  "));


In cat you used process.stdout.write and here you're using console.log - what's the difference? When would you choose to use one or the other?

We use process.stdout.write when we need full control over spacing and newlines, byte-exact output matters, and to avoid automatic newlines, like in cat.
We use console.log when we just want simple, line‑by‑line output with automatic newlines, like in ls.

illicitonion · 2026-03-16T13:03:44Z

implement-shell-tools/ls/ls.js

+      console.log(names.join("  "));
+    }
+  } catch (error) {
+    console.error(`Error reading directory ${dirPath}: ${error.message}`);


What exit code should our process terminate with if it hit an error? What does your code do?

illicitonion · 2026-03-16T13:04:23Z

implement-shell-tools/ls/ls.js

+  if (directories.length === 0) {
+    directories = [process.cwd()];
+  }
+  //If a directory is specified, list that directory


This comment seems a bit confusing? I'd remove it

illicitonion · 2026-03-16T13:05:27Z

implement-shell-tools/ls/ls.js

+        if (directories.length > 1 && index < directories.length - 1)
+          console.log("");


I'd recommend always using {}s around if conditions, or perhaps sometimes allowing single-line statements (like your above if (directories.length > 1) console.log(`${arg}:`);) - see https://www.blackduck.com/blog/understanding-apple-goto-fail-vulnerability-2.html for an example of why

illicitonion · 2026-03-16T13:06:18Z

implement-shell-tools/wc/wc.js

+
+      printCounts(file, counts, options);
+    } catch (error) {
+      console.error(`Error reading file ${file}: ${error.message}`);


Same question about exit codes

illicitonion

Looks good! Just a few last things :)

illicitonion · 2026-03-16T14:21:35Z

implement-shell-tools/cat/cat.js

+      process.stdout.write(`${prefix}${line}\n`);
+    });
+  } catch (error) {
+    console.error(`cat: ${filePath}: ${error.message}`);


This will still exit 0 if e.g. a file didn't exist.

illicitonion · 2026-03-16T14:23:09Z

implement-shell-tools/ls/ls.js

+      const stats = fs.statSync(arg);
+
+      if (stats.isDirectory()) {
+        //Print header if multiple directories are listed


Is this comment useful compared to just reading the code?

illicitonion · 2026-03-16T14:23:21Z

implement-shell-tools/ls/ls.js

+  //remove options from args
+  let directories = args.filter((arg) => arg !== "-a" && arg !== "-1");
+
+  // Default to current directory if no directories are specified


Is this comment useful compared to just reading the code?

illicitonion · 2026-03-16T14:25:38Z

implement-shell-tools/cat/cat.js

+  }
+}
+
+function main() {


This tool behaves slightly differently than built-in cat for the examples given in https://github.com/CodeYourFuture/Module-Tools/blob/main/implement-shell-tools/cat/README.md

Your wc does too for the same reason.

Thank you so much for that. I'll write an integration test to find the issue.

implements-shell-tool

708e4ce

This comment has been minimized.

Sign in to view

ebrahimbeiati added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 14, 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 Mar 16, 2026

illicitonion reviewed Mar 16, 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 Mar 16, 2026

illicitonion mentioned this pull request Mar 16, 2026

Mirror of 376 CodeYourFuture/Module-Tools-Mirror-For-AI-Code-Reviewer-Testing#2

Open

applied the feedback

972ae2b

ebrahimbeiati 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 Mar 16, 2026

illicitonion reviewed Mar 16, 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 16, 2026

ebrahimbeiati added 2 commits March 16, 2026 13:47

applied the new feedbacks

09cde63

applied the new feedbacks

643fefa

ebrahimbeiati 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 Mar 16, 2026

illicitonion reviewed Mar 16, 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 16, 2026

		if (directories.length > 1 && index < directories.length - 1)
		console.log("");

Uh oh!

Conversation

ebrahimbeiati commented Mar 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Learners, PR Template

Self checklist

Changelist

Uh oh!

This comment has been minimized.

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ebrahimbeiati commented Mar 14, 2026 •

edited

Loading