-
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; | ||
|
|
||
| const opts = program.opts(); | ||
|
|
||
| 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? |
||
| }; | ||
|
|
||
| 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? |
||
| 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? |
||
|
|
||
| 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? |
||
| 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? |
||
| 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? |
||
| } | ||
|
|
||
| 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? |
||
| 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?