London | 26-SDC-MAR | Jamal Laqdiem | Sprint 3 | Implement Shell Tools by jamallaqdiem · Pull Request #437 · CodeYourFuture/Module-Tools

jamallaqdiem · 2026-03-26T11:06:57Z

Learners, PR Template

Self checklist

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

Changelist

shell tools exercises.

illicitonion · 2026-03-27T17:23:31Z

implement-shell-tools/cat/cat.js

+      }
+    });
+  } catch (error) {
+    console.log(`Could not read: ${path}`);


If there's an error where will this message get printed? stdout or stderr? And what exit code would the process exit with?

Do these match our expectations from shell tools?

Thanks for reviewing, I made the changes from console.log that wil send txt to stdout To console.error that willsend to stderr and display the error in the screen.
Added pocess.exit(1) to make sure that in case of failure the code should exit with a non zero code.

illicitonion · 2026-03-27T17:24:02Z

implement-shell-tools/cat/cat.js

+        console.log(`${counterLines++}  ${line}`);
+      } else if (showNonBlankNumbers) {
+        // increment and show numbers only if the line is not empty.
+        if (line.trim() !== "") {
+          console.log(`${counterLines++}  ${line}`);
+        } else {
+          // print empty lines
+          console.log(line);
+        }
+      } 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.

Thanks for reviewing, I followed your correct instructions , as it's much clearer and readable using a variable.

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

implement-shell-tools/cat/cat.js

+const showNonBlankNumbers = argv.includes("-b");
+
+//filter the - from the array argv as it's a flag.
+const filePaths = argv.filter((arg) => !arg.startsWith("-"));


What would happen if someone passed a -q flag here? What should happen?

Thanks for reviewing, you are right to point that out, my logic will let the user confused thinking that the -q flag exists and worked, I implemented in place a flag check and ensured print an error and exit with not 0 code.

illicitonion · 2026-03-27T17:25:58Z

implement-shell-tools/ls/ls.js

+const showHiddenFiles = argv.includes("-a");
+
+// if no folder provide we use the current one
+const target = filePaths[0] || ".";


What would happen if someone specified multiple paths, e.g. ls /some/file /some/other/file? What does your implementation do?

The README.md only requires that your programme works with simple paths, but I would recommend implementing support for multiple. But if you don't implement that, you generally want to give an error to the user if they supply input you don't expect, rather than just ignoring it.

Thanks for reviewing, I should at least display an stderr and exit .

illicitonion · 2026-03-27T17:26:50Z

implement-shell-tools/wc/wc.js

+filePaths.forEach((filePath) => {
+  const content = fs.readFileSync(filePath, "utf-8");
+
+  const lines = content.split("\n").length - 1;


Why do you have to - 1 here?

Thanks for reviewing, I used -1 because my vscode automatically add a new line when save, so in this case split method will add an empty element, however in other environments this logic my fail if no trialling new line added .

We may add a conditional check using .pop method in case of empty string at the end

illicitonion · 2026-03-27T17:28:24Z

implement-shell-tools/wc/wc.js

+
+  let output = "";
+
+  if (showLines || noFlags) output += `${lines} `;


Can you think of a way to achieve this same result, but without needing a noFlags variable at all?

If our program was bigger and more complicated, in most of the program we don't want to have to think about what the exact flags interface was. Ideally we can just look at showLines to decide whether we should show lines, without needing to worry which exactly flags led to us needing to show lines.

Thanks for reviewing, I implemented a clear way to switch all flags to true if no flag was picked.
Fixed a bug.

jamallaqdiem added 3 commits March 21, 2026 08:46

implementing a logic that behive as cat command

276fc2b

Solving The exercise for ls command.

8675a8b

feat: create a script that behave like wc command.

99cb233

jamallaqdiem added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Module-Tools The name of the module. 📅 Sprint 3 Assigned during Sprint 3 of this module labels Mar 26, 2026

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

FIx: Implement the missing logics.

e1d7c80

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


		let output = "";

		if (showLines \|\| noFlags) output += `${lines} `;

Uh oh!

Conversation

jamallaqdiem commented Mar 26, 2026

Learners, PR Template

Changelist

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants