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

+      }
+    });
+  } 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

+        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

+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

+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

+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

Please do the conditional pop check :)

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

+
+  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.

illicitonion · 2026-04-22T12:53:38Z

+// THis will give an array without the path to node and to the file.
+const argv = process.argv.slice(2);
+
+//Get line numbers.


Does this comment provide value beyond the variable names?

If yes, could you change the variable names to make this comment no longer necessary?

illicitonion · 2026-04-22T12:54:14Z

+const supportedFlags = ["-n", "-b"];
+for (const flag of flagsUsed) {
+  if (!supportedFlags.includes(flag)) {
+    console.error(`Invalid option try 'node cat.js --help' for more info.`);


What does node cat.js --help do? As far as I can tell, it will probably print this error?

illicitonion · 2026-04-22T12:55:28Z

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


Right now you're always printing the same error message if something went wrong.

What are the advantages/disadvantages to including the specific error message in the output to the user?

The advantages is that will show the users the error and allow him to adjusted themselves.
The disadvantages is that it may leak sensitive data like a name in the path.

Yeah, this is generally correct, though specific on names in the path, the user presumably has access to the name in the path because if they didn't they wouldn't have been able to pass it as an argument.

illicitonion · 2026-04-22T12:57:33Z

+// read the file.
+const files = fs.readdirSync(target);
+
+// save the result into the variable.


Almost all of your comments provide no value over just reading the code. Comments should explain why we're doing something (if it's not obvious), or explain something particularly confusing/tricky. If you can read the code, the comments here aren't useful - please consider all of them and remove any you think aren't useful.

illicitonion · 2026-04-22T12:58:12Z

+}
+// fix bug .length ===0 will be true if nothing provided, instead !filePath will return empty array which result true.
+if (filePaths.length === 0) {
+  console.error("PLease provide a file path");


You frequently have typos where the second letter after a capital letter is also capitalised. Please look out for these and fix them.

illicitonion · 2026-04-22T12:59:03Z

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


Please do the conditional pop check :)

illicitonion · 2026-04-22T13:00:00Z

+  if (showCharacters) output += `${characters} `;
+
+  console.log(`${output} ${filePath}`);
+});


Your output doesn't match real wc's output if multiple files were passed. Please make sure to run the examples given in the READMEs and that your behaviours match.

jamallaqdiem · 2026-04-25T08:20:20Z

Thanks for the review, I addressed all matters.

illicitonion

Looks much better, well done!

illicitonion · 2026-04-27T14:39:58Z

+      if (showNonBlankNumbers) {
+        if (line.trim() !== "") {
+          prefix = `${counterLines++}  `;
+        }
+      } else if (showNumbers) {
+        prefix = `${counterLines++}  `;
+      }


I would think about collapsing this into one if:

if (showNumbers || (howNonBlankNumbers && line.trim() !== "")) {

because they have the same intent, but wouldn't push strongly for this if you prefer it as-is :)

illicitonion · 2026-04-27T14:40:13Z

+
+const files = fs.readdirSync(target);
+
+let filteredFIles = files;


Suggested change

let filteredFIles = files;

let filteredFiles = files;

illicitonion · 2026-04-27T14:41:27Z

+
+if (filePaths.length > 1) {
+  let totalOutput = "";
+  if (showLines)  totalOutput += `${totalLines.toString().padStart(8)}`;


This is a bit repetitive with the single-file formatting case :)

illicitonion · 2026-04-27T14:42:19Z

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


Yeah, this is generally correct, though specific on names in the path, the user presumably has access to the name in the path because if they didn't they wouldn't have been able to pass it as an argument.

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

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

fixed the errors.

2bec857

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

illicitonion reviewed Apr 27, 2026

View reviewed changes


		let output = "";

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


		const files = fs.readdirSync(target);

		let filteredFIles = files;

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!

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!

jamallaqdiem commented Apr 25, 2026

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