-
Notifications
You must be signed in to change notification settings - Fork 2
London | 25-SDC-July | Andrei Filippov | Sprint 3 | Implement shell tools #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| import { promises as fs } from "node:fs"; | ||
| import { program } from "commander"; | ||
|
|
||
| program | ||
| .name("cat") | ||
| .description("Concatenate and print files") | ||
| .option("-n", "Number the output lines, starting at 1") | ||
| .option("-b", "Number the non-blank output lines, starting at 1") | ||
| .argument("<sample-files...>", "The file path to process") | ||
| .parse(); | ||
|
|
||
| const argv = program.args; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see that 'opts' and 'argv' are declared outside of any function. While this works, do you think there might be benefits to limiting their scope to where they're actually used? Keeping variables as close as possible to where they're needed can help prevent accidental changes and make your code easier to follow. |
||
|
|
||
| const opts = program.opts(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see that 'opts' and 'argv' are declared outside of any function. While this works, do you think there might be benefits to limiting their scope to where they're actually used? Keeping variables as close as possible to where they're needed can help prevent accidental changes and make your code easier to follow. |
||
|
|
||
| const countLines = (data) => { | ||
| const lines = data.split("\n"); | ||
| if (lines[lines.length - 1] === "") { | ||
| lines.pop(); | ||
| } | ||
|
|
||
| let lineNum = 1; | ||
|
|
||
| for (const line of lines) { | ||
| if (opts.b) { | ||
| if (line.trim() === "") { | ||
| console.log(); | ||
| } else { | ||
| console.log(`${lineNum} ${line}`); | ||
| lineNum++; | ||
| } | ||
| } else if (opts.n) { | ||
| console.log(`${lineNum} ${line}`); | ||
| lineNum++; | ||
| } | ||
| } | ||
|
Comment on lines
+24
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When you have two code sections that are very similar except for a small difference, is there a way you could combine them? What would that look like here?
Comment on lines
+24
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When two different options lead to almost the same behavior, what pattern could you use to reduce duplicated code and keep it easier to update later? |
||
| }; | ||
|
Comment on lines
+16
to
+37
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've noticed that both the '-b' and '-n' options in your code use very similar logic to number lines, with only a small difference in how blank lines are handled. Do you think you could combine this logic into a single function, perhaps by passing in a parameter to control whether blank lines are numbered? This might help reduce repetition and make your code easier to maintain. |
||
|
|
||
| async function example(path) { | ||
| try { | ||
| const data = await fs.readFile(path, { encoding: "utf8" }); | ||
| if (opts["b"]) { | ||
| countLines(data); | ||
| } else if (opts["n"]) { | ||
| countLines(data); | ||
| } else { | ||
|
Comment on lines
+41
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When two different options lead to almost the same behavior, what pattern could you use to reduce duplicated code and keep it easier to update later?
Comment on lines
+42
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a case where both -b and -n could be set? What does the standard tool do if this happens? Could you adjust your logic to handle this in a way that would make sense to users?
Comment on lines
+42
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've noticed that both the '-b' and '-n' options in your code use very similar logic to number lines, with only a small difference in how blank lines are handled. Do you think you could combine this logic into a single function, perhaps by passing in a parameter to control whether blank lines are numbered? This might help reduce repetition and make your code easier to maintain. |
||
| console.log(data.trimEnd()); | ||
| } | ||
| } catch (err) { | ||
| console.error(err); | ||
| } | ||
| } | ||
|
Comment on lines
+39
to
+52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How do you usually choose names for your functions and variables? What could you rename 'example' and 'argv' to make their purposes clearer, both to you in the future and to someone else reading your code?
Comment on lines
+39
to
+52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How do you decide on meaningful names for your functions? Can you think of a name that might better communicate this function's purpose?
Comment on lines
+39
to
+52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there another function name that would help a future reader quickly understand what this function does? What about something like 'processFile'? |
||
|
|
||
| const handleInput = async () => { | ||
| for (const path of argv) { | ||
| await example(path); | ||
| } | ||
| }; | ||
|
|
||
| handleInput(); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| import { promises as fs } from "node:fs"; | ||
| import process from "node:process"; | ||
| import path from "node:path"; | ||
| import { program } from "commander"; | ||
|
|
||
| program | ||
| .name("ls") | ||
| .description("Shows files in directory") | ||
| .option("-1", "list one file per line") | ||
| .option( | ||
| "-a", | ||
| "Used to list all files, including hidden files, in the current directory" | ||
| ) | ||
| .argument("[sample-files]", "The file path to process"); | ||
|
|
||
| program.parse(); | ||
|
|
||
| let pathToFile = ""; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you spot a way to limit the scope of 'pathToFile' so it's only available where it's actually used? What changes would that require in your structure? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you think keeping variables inside the smallest possible scope is a helpful practice? Can you imagine how this variable could be kept within the async block or passed as a parameter instead of being an outer variable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What would be the effect of declaring pathToFile inside your main async function, instead of at the top? Would this make it clearer where it's used and how it's set? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The variable 'pathToFile' is declared at the top and then reassigned in different branches of your code. What do you think about declaring it closer to where it's used, or even passing it as a parameter to functions that need it? This can help avoid confusion about its value at different points in your program. |
||
|
|
||
| const programArgv = program.args; | ||
|
|
||
| (async () => { | ||
| if (programArgv.length === 1) { | ||
| pathToFile = programArgv[0]; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you spot a way to limit the scope of 'pathToFile' so it's only available where it's actually used? What changes would that require in your structure? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you think keeping variables inside the smallest possible scope is a helpful practice? Can you imagine how this variable could be kept within the async block or passed as a parameter instead of being an outer variable? |
||
| try { | ||
| const stats = await fs.stat(pathToFile); | ||
| if (stats.isFile()) { | ||
| await listFiles("file"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you think keeping variables inside the smallest possible scope is a helpful practice? Can you imagine how this variable could be kept within the async block or passed as a parameter instead of being an outer variable? |
||
| } else if (stats.isDirectory()) { | ||
| listFiles("directory"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you think keeping variables inside the smallest possible scope is a helpful practice? Can you imagine how this variable could be kept within the async block or passed as a parameter instead of being an outer variable? |
||
| } else { | ||
| console.error("Not a file or directory."); | ||
| } | ||
| } catch (err) { | ||
| console.error("Invalid path:", err.message); | ||
| } | ||
| } else if (programArgv.length === 0) { | ||
| pathToFile = process.cwd(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you spot a way to limit the scope of 'pathToFile' so it's only available where it's actually used? What changes would that require in your structure? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you think keeping variables inside the smallest possible scope is a helpful practice? Can you imagine how this variable could be kept within the async block or passed as a parameter instead of being an outer variable? |
||
| await listFiles("directory"); | ||
| } else { | ||
| console.error( | ||
| `Expected no more than 1 argument (sample-files) but got ${programArgv.length}.` | ||
| ); | ||
| } | ||
| })(); | ||
|
Comment on lines
+23
to
+45
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a way you could structure your code so the decision of whether to list files, directories, or error messages, and how to display them, is handled in just one spot? Why might that make maintenance simpler over time? |
||
|
|
||
| const flag_1 = (files) => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you explained what each function and variable does to a friend who had never seen the code, what might you name them instead so they're easy to understand? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed that you use variable and function names like 'char' and 'flag_1'. Do these names clearly describe what the variable or function does? For example, 'char' actually holds the parsed options, and 'flag_1' prints files one per line. Choosing more descriptive names can make your code easier for others (and your future self) to understand. |
||
| files.forEach(function (file) { | ||
| console.log(file); | ||
| }); | ||
| }; | ||
|
Comment on lines
+47
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When you name your functions, how do you decide on a name? What might be some more descriptive names for flag_1? |
||
|
|
||
| const flag_a = (files) => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you explained what each function and variable does to a friend who had never seen the code, what might you name them instead so they're easy to understand? |
||
| files.unshift(".."); | ||
| files.unshift("."); | ||
| return files; | ||
| }; | ||
|
Comment on lines
+53
to
+57
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How could you ensure that you're not duplicating '.' and '..' entries if flag_a gets called several times on the same array? What do you think would happen currently if someone tried that? |
||
|
|
||
| async function listFiles(type) { | ||
| let output = []; | ||
| let formattedPath = ""; | ||
| if (type == "directory") { | ||
| formattedPath = pathToFile; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you think keeping variables inside the smallest possible scope is a helpful practice? Can you imagine how this variable could be kept within the async block or passed as a parameter instead of being an outer variable? |
||
| } else if (type == "file") { | ||
| formattedPath = path.dirname(pathToFile); | ||
|
Comment on lines
+63
to
+65
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you spot a way to limit the scope of 'pathToFile' so it's only available where it's actually used? What changes would that require in your structure? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you think keeping variables inside the smallest possible scope is a helpful practice? Can you imagine how this variable could be kept within the async block or passed as a parameter instead of being an outer variable? |
||
| } | ||
| const char = program.opts(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you explained what each function and variable does to a friend who had never seen the code, what might you name them instead so they're easy to understand? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed that you use variable and function names like 'char' and 'flag_1'. Do these names clearly describe what the variable or function does? For example, 'char' actually holds the parsed options, and 'flag_1' prints files one per line. Choosing more descriptive names can make your code easier for others (and your future self) to understand. |
||
| const files = await fs.readdir(formattedPath); | ||
| const sortedOutput = files.sort((a, b) => a.localeCompare(b)); | ||
|
|
||
| if (char["a"]) { | ||
| output = flag_a(sortedOutput); | ||
| } else { | ||
| sortedOutput.forEach(function (file) { | ||
| if (file[0] != ".") { | ||
| output.push(file); | ||
| } | ||
| }); | ||
|
Comment on lines
+74
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you encountered array filter methods before? How could you use them to make this block of code shorter or clearer? |
||
| } | ||
|
Comment on lines
+68
to
+79
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In your 'listFiles' function, you sort and filter files in a couple of places. Do you think you could extract this logic into a helper function? This might help reduce repetition and make it easier to update the logic in the future. |
||
|
|
||
| if (char["1"]) { | ||
| flag_1(output); | ||
| } else { | ||
| console.log(output.join(" ")); | ||
| } | ||
| } | ||
|
Comment on lines
+59
to
+86
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a way you could structure your code so the decision of whether to list files, directories, or error messages, and how to display them, is handled in just one spot? Why might that make maintenance simpler over time?
Comment on lines
+59
to
+86
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think a better name for the function that handles both files and directories could be? Does calling it 'listFiles' match what you expect it to do when reading code? |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| import { program } from "commander"; | ||
| import { promises as fs } from "node:fs"; | ||
|
|
||
| program | ||
| .name("count-containing-words") | ||
| .description("Counts words in a file that contain a particular character") | ||
| .option( | ||
| "-l", | ||
| "The number of lines in each input file is written to the standard output." | ||
| ) | ||
| .option( | ||
| "-w", | ||
| "The number of words in each input file is written to the standard output." | ||
| ) | ||
| .option( | ||
| "-c", | ||
| "The number of bytes in each input file is written to the standard output." | ||
| ) | ||
| .argument("<path...>", "The file path to process") | ||
| .parse(); | ||
|
|
||
| const argv = program.args; | ||
|
|
||
| const opts = program.opts(); | ||
|
|
||
| const total = []; | ||
| const output = []; | ||
| const countsPerFile = []; | ||
|
Comment on lines
+26
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you see a way to combine or clarify these arrays so it's obvious what each one does, or maybe structure your results a little differently for clarity and future flexibility?
Comment on lines
+26
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Names like 'total' and 'output' are a bit generic. For example, 'total' actually holds the sum of counts for all files, and 'output' is a list of results for each file. Do you think more specific names could help clarify what these variables represent? |
||
| let columnWidth = 0; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What would happen if you moved the declaration of 'columnWidth' inside 'handleInput' instead of declaring it outside? How might that affect readability and maintainability for you or someone else looking at your code? |
||
|
|
||
| const flag_c = (content) => { | ||
| return Buffer.byteLength(content, "utf8"); | ||
| }; | ||
|
|
||
| const flag_w = (content) => { | ||
| return content.match(/\b[\w']+\b/g).length; | ||
| }; | ||
|
|
||
| const flag_l = (content) => { | ||
| return content.split("\n").length - 1; | ||
| }; | ||
|
Comment on lines
+39
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How could you ensure that the count matches typical behavior, especially for files that may or may not end with a newline? Could you test with different kinds of files and see what outputs you get? |
||
|
|
||
| const countAndDisplay = async (path) => { | ||
| const outputPerFile = []; | ||
| const content = await fs.readFile(path, "utf-8"); | ||
| if (opts["l"]) { | ||
| outputPerFile.push(flag_l(content)); | ||
| } | ||
| if (opts["w"]) { | ||
| outputPerFile.push(flag_w(content)); | ||
| } | ||
| if (opts["c"]) { | ||
| outputPerFile.push(flag_c(content)); | ||
| } | ||
|
Comment on lines
+46
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you look for patterns in the way you're handling each of the '-l', '-w', and '-c' options, can you imagine a way to combine the operations to make your code shorter and easier to change in the future?
Comment on lines
+46
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What would happen if you had to add more options like -m for characters? Can you imagine a way to pair up the options with their processing functions so the code to handle them doesn't need to be written out every time? |
||
| if (argv.length > 1) { | ||
| if (total.length == 0) { | ||
| total.push(...outputPerFile); | ||
| } else { | ||
| for (let index = 0; index < outputPerFile.length; index++) { | ||
| total[index] += outputPerFile[index]; | ||
| } | ||
| } | ||
| countsPerFile.push(...outputPerFile); | ||
| } | ||
| outputPerFile.push(path); | ||
| output.push([...outputPerFile]); | ||
| }; | ||
|
Comment on lines
+43
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you wanted to adapt this code to work differently (for example, outputting results in a different format), how might breaking this apart into smaller functions make it easier to change? |
||
|
|
||
| const handleInput = async () => { | ||
| if (Object.keys(opts).length == 0) { | ||
| ["l", "w", "c"].forEach((key) => (opts[key] = true)); | ||
| } | ||
| for (const path of argv) { | ||
| await countAndDisplay(path); | ||
| } | ||
| const numOfColumns = Object.keys(opts).length; | ||
| if (argv.length > 1) { | ||
| total.push("total"); | ||
| output.push(total); | ||
| } | ||
| for (const row of output) { | ||
| for (let i = 0; i < numOfColumns; i++) { | ||
| columnWidth = Math.max(columnWidth, String(row[i]).length); | ||
| } | ||
| } | ||
|
|
||
| for (let row of output) { | ||
| const line_parts = []; | ||
| for (let i = 0; i < numOfColumns; i++) { | ||
| line_parts.push(String(row[i]).padStart(columnWidth + 4)); | ||
| } | ||
|
Comment on lines
+81
to
+91
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What would happen if you moved the declaration of 'columnWidth' inside 'handleInput' instead of declaring it outside? How might that affect readability and maintainability for you or someone else looking at your code? |
||
| console.log(line_parts.join(" "), row.at(-1)); | ||
| } | ||
| }; | ||
| handleInput(); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you usually choose names for your functions and variables? What could you rename 'example' and 'argv' to make their purposes clearer, both to you in the future and to someone else reading your code?