London | Nadika Zavodovska | Module-Tools | Sprint 3 | Implement-Shell-Tools by nadika-zavodovska · Pull Request #45 · CodeYourFuture/Module-Tools

nadika-zavodovska · 2025-03-14T12:10:56Z

Hello,

I've done all the tasks for the Implement-Shell-Tools.

Self checklist

I have committed my files one by one, on purpose, and for a reason
I have titled my PR with COHORT_NAME | FIRST_NAME LAST_NAME | REPO_NAME | WEEK
I have tested my changes
My changes follow the style guide
My changes meet the requirements of this task

Thank you.

ehwus

This is a great submission Nadika that betrays a good working knowledge of Node scripting as well as the functionality of the shell tools that you've been replicating. There are a few points that I've raised surrounding efficiency - as we're working with files, it's important that we're mindful of duplicating these into memory and the most efficient way to work with them will always be one line at a time, see this article for a guide on how to do that: https://www.geeksforgeeks.org/how-to-read-a-file-line-by-line-using-node-js/

Overall everything was very clear and readable and works when running it locally, keep it up!

ehwus · 2025-03-29T12:18:09Z

+            return line;
+        });
+        // join lines back to the string and print them
+        console.log(arrayContentLines.join("\n"));


question: Do we need to join these back, or could we just print each line as we go?

Thank you for the really useful comment, @ehwus Alex! You’re absolutely right, printing each line as we go is more efficient, especially when dealing with large files. I’ve updated my code to avoid unnecessary memory usage.

ehwus · 2025-03-29T12:19:03Z

+//Get user inputs from command line
+const filePaths = program.args;
+const displayLineNumbers = program.opts().n;
+const displayNonEmptyLineNumbers = program.opts().b;


praise: It's great to assign these options into variables that are more descriptive than the single letter flags, it makes the code much more readable

Thank you for the positive feedback, @ehwus Alex. I'll continue to follow this approach in the future!

ehwus · 2025-03-29T12:26:26Z

+}
+
+// Handling multiply file processing using Promise.all and map()
+await Promise.all(arrayFilePaths.map(filePath => displayFileContents(filePath, displayLineNumbers, displayNonEmptyLineNumbers)));


question: Why is map useful here over forEach?

Thank you, @ehwus Alex, great question. map is used because it returns an array of promises, which is needed for Promise.all to wait for all tasks to complete. forEach doesn't return anything, so it wouldn't work with Promise.all.

ehwus · 2025-03-29T12:33:02Z

+    .parse(process.argv)
+
+// Function to print file contents with the flags
+async function displayFileContents(filePath, displayLineNumbers = false, displayNonEmptyLineNumbers = false) {


issue: This function works and is readable, but we're loading the same data into memory multiple times in different ways. Can you think of any ways that this could be made more efficient?

@ehwus Alex, thank you for the really useful feedback! Yes, there is a more efficient way, I can use the readline module. I've updated the code to use readline to read the file line by line, which helps prevent loading the entire file into memory at once.

ehwus · 2025-03-29T12:44:03Z

+// Importing the 'program' object from 'commander' to help build command-line interfaces, handle user inputs.
+import { program } from "commander";
+// Importing the 'chalk' module to add colors and styles to text in the console
+import chalk from "chalk";


praise: 🌈

ehwus · 2025-03-29T12:56:41Z

+        // Array stores the names of the files and directories
+        let displayFiles = [];
+        // Ensure '.', '..' are on the top of list when we list hidden files
+        if (showHiddenFiles) {


praise: This is a clever way of adding the dot directories

ehwus · 2025-03-29T14:38:07Z

+            );
+        });
+        // Array with style directory in bold and blue
+        const styledAllFiles = await Promise.all(allFiles);


praise: Smart awaiting of Promises here

ehwus · 2025-03-29T14:39:49Z

+        // Split content into array by lines
+        let arrayLines = content.split("\n");
+        // If the last line is empty - remove it
+        while (arrayLines.length > 0 && arrayLines[arrayLines.length - 1].trim() === "") {


question: Do we really need a while loop to achieve the result that we're looking for in the comment?

You are right, we can simply filter out empty lines at the end by using arrayLines.filter(line => line.trim() !== "") without needing the while loop. I have updated the code. Thank you, @ehwus Alex!

ehwus · 2025-03-29T14:43:34Z

+        }
+
+        // Get the number of lines 
+        const countNumLines = arrayLines.length;


nitpick: Do we always need to know the results of each of these?

You're right, it's usually better to keep the code concise and avoid unnecessary variables. In this case, I used a variable because I needed the same value in two places, so it helped avoid repeating the calculation. But I agree, that keeping the code clean and minimal is a good practice.

ehwus · 2025-03-29T14:44:06Z

+    .parse(process.argv);
+
+// Function to count bytes, words and lines in a file
+async function countDisplayNumBytesWordsLines(filePath, numLines = false, numWords = false, numBytes = false) {


praise: This function encapsculates the core logic well

nadika-zavodovska · 2025-04-08T21:05:02Z

@ehwus Alex, thank you for your insightful and encouraging feedback, and the helpful link! I read through the article, and it was really useful for learning how to work with files one line at a time and manage memory more efficiently. I appreciate your support in writing cleaner and more maintainable code.

nadika-zavodovska added 4 commits March 13, 2025 10:16

Implement ls command

d3fa55f

Implement cat command

ff16c21

Implement wc command

dc096f1

Add package.json

2ab48d1

nadika-zavodovska 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 14, 2025

nadika-zavodovska mentioned this pull request Mar 14, 2025

01 Implement shell tools (cat, ls, wc) in JavaScript with NodeJS nadika-zavodovska/My-Coursework-Planner#158

Open

Refactor

509a5db

ehwus 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 29, 2025

ehwus reviewed Mar 29, 2025

View reviewed changes

ehwus 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 29, 2025

nadika-zavodovska added 4 commits April 8, 2025 18:25

Remove join method, implement print as we go in loop

8ed1366

Change to readline module

5d98dfd

Refactor

a1bec5b

Refactor

a33f391

Uh oh!

Conversation

nadika-zavodovska commented Mar 14, 2025

Uh oh!

ehwus 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!

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!

nadika-zavodovska commented Apr 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants