HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Counting newlines in a file

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
filecountingnewlines

Problem

I've started to go through Node tutorials on nodeschool.io, and the second assignment was to write a program that will count the number of newlines in a file and print it out. My solution looks like this

var fs = require('fs');

var file = process.argv[2];
var buff = fs.readFileSync(file);
var str = buff.toString();
var str_arr = str.split('\n');
var newline_length = str_arr.length-1;

console.log(newline_length);


It's correct, and works. Theirs is

var fs = require('fs')

var contents = fs.readFileSync(process.argv[2])
var lines = contents.toString().split('\n').length - 1
console.log(lines)

// note you can avoid the .toString() by passing 'utf8' as the
// second argument to readFileSync, then you'll get a String!
//
// fs.readFileSync(process.argv[2], 'utf8').split('\n').length - 1


While both are correct, mine is longer (more lines used because I declared variables at every step).

I am wondering, am I wrong in declaring the variables for every step and then working on the code as I go? I mean, it does take more space, but I have the feeling that it's a bit clearer what I'm doing, unlike theirs. Or am I mistaken and haven't really gotten to that level where I can safely write everything in a one line like they did?

Also am I using memory space by declaring too many variables?

This task is rather trivial, and mostly the code I've written so far is for web elements (carousels and whatnot), so I'm used to writing mostly jQuery code instead of pure JavaScript (declaring variables used in events and such). Does this have any impact in real life apps?

Solution

The two versions execute exactly the same steps,
the only difference is the number of intermediary local variables.

Which version is more clear can be a matter of opinion.
Beginners may find your verbose version more clear,
more experienced programmers may find it tedious.

The good thing about the second version is that the local variables mark bigger logical steps that are quite intuitive. With one weakness, the meaning of the lines variable is not immediately clear. If you say "lines", it might mean a collection, or the count of lines.
But actually it's neither, it's the number of newline characters.

By contrast, your version marks the steps in too much detail,
for example it's not particularly interesting that readFileSync(file) doesn't return a String and it needs to be converted.
That step could be collapsed, as it is a too specific implementation detail,
not something "logical".

I think the best of both worlds would be more like this:

var fs = require('fs');

var path = process.argv[2];    
var text = fs.readFileSync(path).toString();
var lines = text.split('\n');
var newlines_count = lines.length - 1;
console.log(newlines_count);


Each step marks a bigger logical step, with slightly more natural variable names.

Code Snippets

var fs = require('fs');

var path = process.argv[2];    
var text = fs.readFileSync(path).toString();
var lines = text.split('\n');
var newlines_count = lines.length - 1;
console.log(newlines_count);

Context

StackExchange Code Review Q#131143, answer score: 8

Revisions (0)

No revisions yet.