Code comments solicited

[Christopher Benson-Manica]

This is intended to be a simple version of the Unix "head" command,
i.e. a utility that displays the first n lines of a file. Comments
welcomed...

#include <cstdlib>
#include <iostream>
#include <fstream>
#include <sstream>
#include <string>

using namespace std;

int main( int argc, char *argv[] )
{
if( argc < 2 || argc > 3 ) {
// endl preferable to "\n" here?
cerr << "Usage: " << argv[0] << " <file> [lines]" << endl;
return EXIT_FAILURE;
}
ifstream f( argv[1] );
unsigned int count;
if( argc == 3 ) {
// as suggested in responses to my question about atoi()
(stringstream(argv[2])) >> count; // no error checking!
}
else {
count=10;
}
if( !f ) {
// endl preferable to "\n" here?
cerr << "Could not open file: " << argv[1] << endl;
return EXIT_FAILURE;
}
string s;
while( !f.eof() && count-- ) {
getline( f, s );
// "\n" preferable?
cout << s << "\n";
}
f.close();
return EXIT_SUCCESS;
}

 

[Karl Heinz Buchegger]

[snip]
string s;
while( !f.eof() && count-- ) {
getline( f, s );

eof is not intended to be used the way you use it here.

The pattern to read a file in C++ is:

while ( read_as_long_as_there_is_no_error ) {
do somthing with the thing read
}

what was the error? if it was eof, then everything is fine.

-------------------------

In your case there is no need to check for eof after
the loop, because not having read to the end of loop
is fine and expected.

Thus:

while( count-- && getline( f, s ) )
cout << s << '\n';

//
// normally you would do
//
// if( !f.eof() )
// cout << "There was an error reading the file\n";
//


Also: you don't check if the conversion from argv[2] into an
int has worked. But I'm sure you already know this.

 

[Christopher Benson-Manica]

while( count-- && getline( f, s ) )
cout << s << '\n';

// if( !f.eof() )
// cout << "There was an error reading the file\n";

Also: you don't check if the conversion from argv[2] into an
int has worked. But I'm sure you already know this.

Yes, I indicated this in a comment

 

[Sharad Kala]

<denseness>Wait, explain again why this isn't needed...?</denseness>

Because the error bit in stream is set after reading the EOF.
Thus your program runs one extra iteration.

Best wishes,
Sharad

 

[Chris Theis]

<denseness>Wait, explain again why this isn't needed...?</denseness>

Your original code looked like this:

while( !f.eof() && count-- ) {
getline( f, s );
}

This actually introduces a kind of unrequired redundancy because you imply
that the contents of the loop is to be executed if count has not yet reached
the lower limit OR the end of file has not yet been reached. If both are
true you attempt to read from the file. Getline actually returns a reference
to the input stream which has an overloaded conversion for void* and bool.
These conversion functions test for the failbit which has been automatically
set if EOF has occured. Thus you can implicitely use the getline statement
to check for EOF and read from the file if EOF has not occured.

Hence,

while( getline(f, s) && count-- ) {
.....
}

do the trick.

Also: you don't check if the conversion from argv[2] into an
int has worked. But I'm sure you already know this.

Yes, I indicated this in a comment

Instead of just commenting on this issue I'd recommend to solve it, for if
there is one thing a developer can expect then it is the unexpected. A
possible way to test for the result of a string conversion is to use the
following function:

////////////////////////////////////////////////////////////////////////////
//
template<typename T>
T FromStringEx( const std::string& s, bool& bSuccess)
// string conversion from string to typename T with a flag
// the represents success or failure.
//
// e.g: int d = FromString<int>( s );
// e.g: string s = ToString( d );
//
// if you want to check whether the conversion was successful then you can
// use the bSuccess flag!
////////////////////////////////////////////////////////////////////////////
//
{
bSuccess = true;
std::istringstream is(s);
T t;
is >> t;
if( !is )
bSuccess = false;
return t;
}

Regards
Chris

 

[Karl Heinz Buchegger]

<denseness>Wait, explain again why this isn't needed...?</denseness>

OK.

First of all:
A stream enters eof state *after* it has tried (and failed) to read
past the end of file. Thus a loop like:

while( !f.eof() ) {
getline ...
do something with the read line
}

will not work. Only after getline has determined that eof was reached,
the next call to eof will return true. But then it's to late, you already
have tried to 'do something with the read line', although there wasn't
a line read.

So what you can do is:

while( !f.eof() ) {
getline ...
if( !f.eof() ) {
do something with the read line
}
}

That will work. But, it only accounts for eof errors (and is ugly). It does
not account for other errors during the read (eg. corrupt floppy, eg...)

But there is a workaround for this. All the read functions return a status
or something that can be used as a status, indicating: read was successfull
or read was not successfull. Thus you change the above to:

while( getline .... )
do something with the read line

That while loop will terminate when a read fails. Now there are many resons
why a read can fail, one of them is: because the read attempts to read
past eof. So normally you test *after* the loop, why the loop has terminated:

while( getline .... )
do something with the read line

if( ! f.eof() )
there was a reading error. The loop should only terminate
because of eof.

Of course this assumes that the whole file needs to be read. But in your
case this is not the case: you only want to read some n lines from the
file, thus you may or may not reach eof when doing so. Thus the test for
eof alone has no significance (But a combination of the streams state and
eof has!):

if( !f && !f.eof() ) { // if the stream has detected a problem AND
// this problem was not eof, then something
// else must have occoured
cout << "Reading error\n";
}

 

[David Harmon]

if( !f ) {
// endl preferable to "\n" here?
cerr << "Could not open file: " << argv[1] << endl;

if (!f)
perror(argv[1]);

 

[Jerry Coffin]

[ ... ]

while( !f.eof() && count-- ) {
getline( f, s );
// "\n" preferable?
cout << s << "\n";
}

I think a for loop is really suitable here:

for (int i=0; i<count && getline(f,s); i++)
std::cout << s << "\n";

IMO, this indicates the real intent a bit better: that you normally
intend to execute a specific number of iterations. You might exit the
loop early, but you really don't expect that as a rule. To make that
even more clear, you _might_ consider something like this:

for (int i=0; i<count; i++) {
if (!getline(f,s))
break;
std::cout << s << "\n";
}

A purist on structure would point out that this breaks the single-entry,
single-exit structure, but IMO, that's more or less a red herring.
Structure is useful to the extent that it improves readability, and if
breaking pure structure doesn't hurt readability (which I would say is
true here) then I wouldn't worry much about it.

 

[David Harmon]

I think a for loop is really suitable here:

for (int i=0; i<count && getline(f,s); i++)
std::cout << s << "\n";

IMO, this indicates the real intent a bit better: that you normally
intend to execute a specific number of iterations. You might exit the
loop early, but you really don't expect that as a rule.

Well, I have to disagree on the style issue. Leaving the loop due to
eof and leaving due to the count are exactly equally normal, expected,
and valid. The decrementing count is well-understood, and adding the
variable i adds unnecessary entities. So, I prefer the while().

Of course as noted by various posters, f.eof() in the condition is bad.
This issue is covered in topic "[15.4] Why does my input seem to process
past the end of file?" of Marshall Cline's C++ FAQ at:
http://www.parashift.com/c++-faq-lite/

 

[Rolf Magnus]

Well, I have to disagree on the style issue. Leaving the loop due to
eof and leaving due to the count are exactly equally normal, expected,
and valid.

The program is intended to read the first 'count' lines, and so the
"normal" thing is to read 'count' lines (a for loop sounds good for
this). However, the loop might stop (i.e. being broken out) earlier if
some error condition (like eof) is met.

The decrementing count is well-understood, and adding the
variable i adds unnecessary entities. So, I prefer the while().

I don't see the connection between those two.

 

[David Harmon]

The program is intended to read the first 'count' lines, and so the
"normal" thing is to read 'count' lines (a for loop sounds good for
this). However, the loop might stop (i.e. being broken out) earlier if
some error condition (like eof) is met.

Or in other words, you agree with Jerry.

I don't see the connection between those two.

I assume the variable i was suggested by the usual for-loop idiom.
Without it, the for loop simplifies down to
for ( ; getline(f,s) && count-- ; )

To me that is simply an uglier way of writing while().

 

[Rolf Magnus]

Or in other words, you agree with Jerry.

Yes. I just wanted to explain the reason for that.

I assume the variable i was suggested by the usual for-loop idiom.
Without it, the for loop simplifies down to
for ( ; getline(f,s) && count-- ; )

To me that is simply an uglier way of writing while().

I was rather thinking of:

for ( ; count; count--)
if (!getline(f, s)) break;

But you're right, it looks a bit odd.

 

[Jerry Coffin]

[ ... ]

Well, I have to disagree on the style issue. Leaving the loop due to
eof and leaving due to the count are exactly equally normal, expected,
and valid.

Here's where we have a fundamental disagreement. I'd guess that well
over 99% of the time people use head, they expect to see exactly the
number of lines specified, NOT fewer. As such, I'd say the early exit
is an unusual enough condition that giving it separate and special
processing is perfectly reasonable.

The decrementing count is well-understood, and adding the
variable i adds unnecessary entities. So, I prefer the while().

A decrementing loop is reasonably well understood, but certainly not
nearly AS well understood or widely used. Adding an "unnecessary
entity" is usually done to make code simpler and easier to understand,
and IMO that's what happens here.

Consider: the primary (ultimately, ONLY) use of typedef is to add
"entities" that are, strictly speaking, unnecessary. Nonetheless, if
you're stuck with declaring a pointer to a function that returns a
pointer to an array of pointers to functions, each of which returns a
pointer to a function that returns an array of int's, you've got two
choices: introduce (at least) one "unnecessary entity", or write lousy
code.

This case isn't nearly that extreme, but in the end you're left with one
thing: if you expect a loop to execute a specific number of times (which
is exactly the case here, regardless of your claim to the contrary) then
you should normally use a for loop. You should use an incrementing
counter unless you have a specific reason to do otherwise, typically
involving using that variable to index into an array, vector, etc.

 

[David Harmon]

Adding an "unnecessary entity" is usually done to make code simpler
and easier to understand, and IMO that's what happens here.

I would certainly agree with the rest if I thought that was the case
here. Few things are worth more in programming than clarity.

Consider: the primary (ultimately, ONLY) use of typedef is to add
"entities" that are, strictly speaking, unnecessary.

This example lifted from a thread in comp.lang.c++.moderated

typedef const char (&r2a)[10];

struct foo {
operator r2a() const; //cannot be declared without the typedef
};

Which says something about C++ syntax.