London| Elhadj Abdoul Diallo| Module-Tools | WEEK3 - Implement-shell-tools by eediallo · Pull Request #25 · CodeYourFuture/Module-Tools

eediallo · 2025-03-05T11:24:47Z

Learners, PR Template

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

…onously

…ng and file path arguments

…t and improve line numbering logic

…ed readability and maintainability

…ng option

…idden files in ls command

…and options

…n a file

…nd character counts for each

…count logic

…s in wc command

…n is enabled

…dling and streamline output logic

…unction

… logic

…function

ehwus

cat

You've produced a readable and working replica of the cat shell tool here, and it shows a solid understanding of node fundamentals and using commander to build scripts. There are a few tweaks that we could make to get this to be as efficient as cat is, and I would focus on:

Learning about operating on files in a memory efficient manner
Why and when to await the response of promises
JavaScript array iteration - which to use when and why

Overall though this is a really solid effort and it was great to see you reach such parity with the cat tool with your own code.

ls

Once again this is a good implementation of ls and the code is clear and readable. There are a few flags for efficiency and clarity, but overall a very good effort.

wc

Once again this largely replicates wc well barring a few little logic tweaks that I have raised, but parts of the code feel complicated to follow and we have also fallen into some of the previously discussed patterns surrounding reading from the filesystem. It feels like it doesn't have the readability of the other two scripts, so I'm curious what the difference was when you wrote it - did you find it a harder problem to solve?

Overall

This is a great demonstration of your knowledge of shell toolings and how to replicate this functionality well in Node, which you were able to do successfully. Keep tradeoffs in mind, particularly when working with the file system, and make sure that you have a good understanding of when to use each array function and when it's useful to await promises.

ehwus · 2025-03-19T07:55:50Z

+  .argument("<paths...>", "The file paths to process")
+  .parse(process.argv);
+
+let args = program.args;


nitpick: Are we ever reassigning this let?

we are not. It makes sense to use the const keyword here.

ehwus · 2025-03-19T07:56:29Z

+  .parse(process.argv);
+
+let args = program.args;
+//console.log(args)


nitpick: It's good practice to leave these debugging comments out of code, but good to see where you were checking your inputs as you went along

ok. going to remove it. Thanks

ehwus · 2025-03-19T07:58:02Z

+let args = program.args;
+//console.log(args)
+
+const nOption = program.opts().number;


suggestion: I appreciate here that cat uses these single letters to denote functionality, but you could make your own life easier here and name them something that's more clear for your own code, i.e. displayLineNumber

will do. Thanks. I was actually struggling to find meaning names for these options.

There are only two hard things in Computer Science: cache invalidation and naming things.

-- Phil Karlton

ehwus · 2025-03-19T08:01:51Z

+
+async function readAndPrintFileContent(path) {
+  try {
+    const content = await fs.readFile(path, { encoding: "utf-8" });


question: What does this do to the file from the filesystem? Could we do this more memory efficiently?

It's reading the content of the file(path) provided through the CLI asyncronously which means it won't prevent other tasks from executing. Could we do this more memory efficiently? I am not 100% sure but I know if I was reading a large file, I would have used Stream to read it in chuncks instead of loading it once.

You're along the right lines here, text files might be absolutely massive (e.g. logs) so making sure that our programs operate on them efficiently is really important, especially as we're processing line by line anyway

ehwus · 2025-03-19T08:03:39Z

+}
+
+function extractLinesFromContent(content) {
+  const lines = content.split("\n");


question: Why could this line of code make the situation that we've introduced on line 22 worse?

I am not sure. Could you please advice what would be the best practice here?

If we've loaded the entire file into memory on line 22, what are we doing here?

we are processing it once.

We're copying the entirety of the file into memory again in an array, so we've doubled our problem

OK. I see. Googling to see how to avoid this.

I have refactored it by removing the extractLinesContent function and extracting lines directly in readAndPrintFileContent function. Would this solve the problem?

ehwus · 2025-03-19T08:31:05Z

+ * @returns {number[]|number} - An array of sums for each column of numbers if there are multiple files,
+ * or 0 if there is only one file.
+ */
+function aggregateFileData(fileCounts) {


question: Please could you talk me through this function line by line?

Sure the aggregatedFilesData is an array with three possible values: sum of lines, chars or words. The following code:

if (aggregatedFilesData !== 0 && lineOption && !(wordOption || charOption)) { console.log(`${aggregatedFilesData[0]} total`); return; }

whether aggregatedFilesData is not 0 and line option is provided but not the rest of the options(chars and words).if it is true, it means only sum of lines is available on aggregatedFilesData array therefore the sum is displayed with total in front of it.

it is the same for chars option in the following code:

if (aggregatedFilesData !== 0 && charOption && !(wordOption || lineOption)) { console.log(`${aggregatedFilesData[0]} total`); return; }

Here:

if ( aggregatedFilesData !== 0 && ((lineOption && charOption) || (lineOption && wordOption)) ) { console.log(`${aggregatedFilesData[0]} ${aggregatedFilesData[1]} total`); return; }

This code checks if two options are provided, it could be line option and char option or line and word option. if it is true, I know the aggregatedFilesData array contains two values which are then displayed alongside the total string.

The return keyword in each statement ensure that the execution does not continue.

if the bove conditions are false: the sum of each option is display if aggregatedFilesData is not 0, hence the code below: aggregatedFilesData !== 0
? console.log(
${aggregatedFilesData[0]} ${aggregatedFilesData[1]} ${aggregatedFilesData[2]} total
)
: null;

ehwus · 2025-03-19T08:43:31Z

+const wordOption = program.opts().word;
+
+async function countLinesWordsCharsInFile(path) {
+  const content = await fs.readFile(path, { encoding: "utf-8" });


question: What are the tradeoffs of reading files in this way?

if the file was large, that would caused high memory consumption and possible out of memory errors.

ehwus · 2025-03-19T08:44:47Z

+async function countLinesWordsCharsInFile(path) {
+  const content = await fs.readFile(path, { encoding: "utf-8" });
+
+  const lines = removeEndEmptyLine(content.split("\n"));


question: These are clever ways of getting the counts, but do we always need to do all of this work?

maybe not. may I could have write the code instead of creating a separate function for it.

I have refactore this code too to avoid copying the entire array inot memory again.

ehwus · 2025-03-19T08:47:05Z

+  const numberOfWords = words.length;
+  const numberOFChars = chars.length;
+
+  if (lineOption) {


question: What if I'd like to have the lines and the chars?

Right. I've not implemented this one. I am going to add the functionality

ehwus · 2025-03-19T08:48:11Z

+}
+
+async function processFilesAndDisplayCounts() {
+  const fileCounts = await Promise.all(args.map(countLinesWordsCharsInFile));


praise: awaiting and using Promise.all with a map here makes a lot of sense and makes sure we can process as many files as are needed, great stuff

…ogic

…ability

…d functionality

… names

… function and handling line extraction directly in readAndPrintFileContent

…on when line and character options are not selected

… improved clarity and remove unnecessary whitespace

[Elhadj Abdoul Diallo] added 30 commits March 3, 2025 11:36

Add implementation for cat shell tool

f5382f7

Refactor cat implementation to support reading multiple files asynchr…

cfa60fe

…onously

Implement display function to output contents of the files

48355e1

Fix variable name for file paths in cat implementation

16944c0

Add package.json and package-lock.json for dependency management

677386b

Enhance cat implementation with command-line options for line numberi…

2fc1242

…ng and file path arguments

Refactor readFiles and displayFileContents functions to flatten outpu…

6c5e0c7

…t and improve line numbering logic

Fix readFiles function to remove trailing empty lines from file contents

fe03c76

Refactor readFiles function to use extractLinesFromContent for improv…

9fb84ad

…ed readability and maintainability

Add option to number non-empty lines in cat implementation

2150905

Update readFiles function to use object syntax for fs.readFile encodi…

2376e64

…ng option

Implement ls command with options for single line output

24ceb1c

Enhance ls command to accept multiple paths as arguments

d823cbe

Add option to include hidden files in ls command

97647ec

Add option to display current and parent directories when including h…

5cc3cac

…idden files in ls command

Remove unnecessary await from displayFileContents function call

6fe4d34

Fix comment wording in extractLinesFromContent function for clarity

80b1b24

Enhance documentation for implementLs function to clarify parameters …

aee2490

…and options

Add wc command implementation to count lines, words, and characters i…

cf568a4

…n a file

Refactor wc command to handle multiple files and return line, word, a…

bad7699

…nd character counts for each

Refactor wc command to handle empty lines correctly and improve word …

d8f858b

…count logic

Fix line count calculation in createLineWordsCharCountForFile function

1239f5f

Add aggregation of line, word, and character counts for multiple file…

0021d1e

…s in wc command

Improve aggregation logic in wc command to handle single file cases

5b3d5c9

Refactor wc command to use commander for argument parsing

2b16d73

Add line count option to wc command

9ddebf4

Add character count option to wc command

e5ccf82

Add word count option to wc command

da2ec96

Fix output logic in wc command to display total count when line optio…

ea33cfd

…n is enabled

Update wc command to conditionally display total count for word option

cd2a6de

[Elhadj Abdoul Diallo] added 4 commits March 5, 2025 10:42

Refactor printFileWithLineNumbers function to improve line number han…

19ed271

…dling and streamline output logic

Rename options for line number handling in printFileWithLineNumbers f…

1517657

…unction

Refactor cat.js to rename and separate file reading and line printing…

6aea96c

… logic

Refactor ls.js to encapsulate directory listing logic in a dedicated …

7c4ca2a

…function

eediallo added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 5, 2025

[Elhadj Abdoul Diallo] added 3 commits March 15, 2025 08:46

ignore .venv. folder

d81c7d8

Add commented debug log for command line arguments in cat.js

c180a48

Remove unused import from cat.js

475f309

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 19, 2025

ehwus reviewed Mar 19, 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 19, 2025

eediallo added 15 commits March 19, 2025 09:46

Use const instead of let for args in cat.js

0ce2ebc

Remove commented debug log for command line arguments in cat.js

bb95ea4

Rename variable nOption to displayLineNumber for clarity in cat.js

417bb33

Rename bOption to displayNonEmptyLineNumber for clarity in cat.js

9e94b59

Refactor printLinesWithOptions to use ternary operators for cleaner l…

148ddae

…ogic

Remove unnecessary await from printLinesWithOptions for improved read…

0f26dac

…ability

Reintroduce function documentation for ls.js to clarify parameters an…

b932eaf

…d functionality

Rename variables for clarity in ls.js and update logic to reflect new…

b75d6f6

… names

Refactor listDirectoryContents in ls.js for improved readability

7ab2cca

Fix character count variable name and enhance output formatting in wc.js

353f36b

Remove unnecessary entry from .gitignore for cleaner configuration

17a4b2d

Simplify file content reading by removing the extractLinesFromContent…

d9d44a6

… function and handling line extraction directly in readAndPrintFileContent

Remove unused function to handle empty line removal directly in wc.js

cd0b63c

Enhance output display in wc.js to include total counts for word opti…

fcf9a71

…on when line and character options are not selected

Refactor directory listing in ls.js to use forEach instead of map for…

05cef26

… improved clarity and remove unnecessary whitespace

ehwus added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Mar 22, 2025

Uh oh!

Conversation

eediallo commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Learners, PR Template

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!

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!

eediallo Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

eediallo commented Mar 5, 2025 •

edited

Loading

eediallo Mar 19, 2025 •

edited

Loading