Mirror of 376#15
Conversation
illicitonion
added
Needs Review
|
Thanks for opening a new PR! AI started to review it. Please notice that AI will review this PR only once |
![cyf-ai-code-reviewer[bot]](https://avatars.githubusercontent.com/u/22743767?s=60&v=4){kind=small}
| if(options.numberNonEmpty) { | ||
| //-b option: number non-empty lines | ||
| if(line.trim()) { | ||
| process.stdout.write( | ||
| `${String(globalLineCounter).padStart(6)}\t${line}\n` | ||
| ); | ||
| globalLineCounter++; | ||
| } else { |
There was a problem hiding this comment.
In the printFile function, the logic that formats and prints numbered lines is repeated for both the -b (number non-empty) and -n (number all) options. In both branches you:
- Convert
globalLineCounterto a string - Pad it to width 6
- Add a tab
- Append the line content
- Then increment
globalLineCounter.
The only real difference between the two branches is when numbering is applied (all lines vs non-empty). When similar formatting logic appears in multiple branches like this, it can make later changes a bit harder: if you decide to adjust the formatting (for example, change the padding or separator), you’d need to remember to update it in every place. How might you extract the shared “print numbered line and increment the counter” behavior into a single helper, and then call it from both the -b and -n paths so that the format is defined in just one place?
To "like" or "dislike" this comment, please follow this link
| function printCounts(filePath, counts, options) { | ||
| const parts = []; | ||
| if(options.line) parts.push(counts.lines); | ||
| if(options.word) parts.push(counts.words); | ||
| if(options.byte) parts.push(counts.bytes); | ||
| //if no specific count options are provided, print all counts | ||
| if(!options.line && !options.word && !options.byte) { | ||
| //default is to print all counts | ||
| parts.push(counts.lines, counts.words, counts.bytes); | ||
| } |
There was a problem hiding this comment.
In printCounts, you’re building up the parts array in two ways: first by checking each option individually (-l, -w, -c), and then again pushing all three counts when no options are provided. This leads to repeating access to counts.lines, counts.words, and counts.bytes in two separate blocks. While this works correctly, it does mean that if you ever wanted to change the order or add another dimension, you’d have to keep both sections in sync. Could you think of a way to describe the available counters and their corresponding options once (for example in a small structure or loop), and then derive both the “only selected fields” and “all fields when none selected” behaviors from that single description?
To "like" or "dislike" this comment, please follow this link
| const filePatterns = []; | ||
|
|
There was a problem hiding this comment.
filePatterns is later used directly as the list of files to read, without any pattern/glob expansion. For a reader, the name filePatterns usually implies something like wildcard patterns (*.txt) that will be expanded or matched, not plain file paths that are used as-is. If someone later tries to add real pattern support, or just reads the code quickly, this name could make them overestimate what this array contains. How might you rename this so that it more clearly communicates that these are the raw file arguments/paths you’re going to open?
To "like" or "dislike" this comment, please follow this link
| const directories = args.filter(arg => arg !== '-a' && arg !== '-1'); | ||
|
|
There was a problem hiding this comment.
The directories array actually holds all non-option arguments, and you then treat each element as either a directory or a file (you call fs.statSync(arg) and may print the name directly if it’s a file). Naming this array directories suggests it only contains directories, which can mislead a reader into thinking there are no files in that list. That can make the later stats.isDirectory() branch a bit surprising to follow. Could a more neutral name that reflects "paths" or "args" make the intent clearer and reduce that confusion?
To "like" or "dislike" this comment, please follow this link
2 participants
No description provided.