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(a rgv[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;
}

--
Christopher Benson-Manica | I *should* know what I'm talking about - if I
ataru(at)cybers pace.org | don't, I need to know. Flames welcome.

[Karl Heinz Buchegger]

Christopher Benson-Manica wrote:

[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_er ror ) {
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.

--
Karl Heinz Buchegger
kb******@gascad .at

[Christopher Benson-Manica]

Karl Heinz Buchegger <kb******@gasca d.at> spoke thus:

while( count-- && getline( f, s ) )
cout << s << '\n'; // if( !f.eof() )
// cout << "There was an error reading the file\n";
<denseness>Wait , explain again why this isn't needed...?</denseness>
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 :)

--
Christopher Benson-Manica | I *should* know what I'm talking about - if I
ataru(at)cybers pace.org | don't, I need to know. Flames welcome.

[Sharad Kala]

"Christophe r Benson-Manica" <at***@nospam.c yberspace.org> wrote in message
news:bu******** **@chessie.cirr .com...

Karl Heinz Buchegger <kb******@gasca d.at> spoke thus:

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

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

<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

[Karl Heinz Buchegger]

Christopher Benson-Manica wrote:

Karl Heinz Buchegger <kb******@gasca d.at> spoke thus:

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

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

<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";
}

--
Karl Heinz Buchegger
kb******@gascad .at

[Chris Theis]

"Christophe r Benson-Manica" <at***@nospam.c yberspace.org> wrote in message
news:bu******** **@chessie.cirr .com...

Karl Heinz Buchegger <kb******@gasca d.at> spoke thus:

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

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

<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<typena me 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::istringstr eam is(s);
T t;
is >> t;
if( !is )
bSuccess = false;
return t;
}

Regards
Chris

[David Harmon]

On Mon, 19 Jan 2004 13:10:06 +0000 (UTC) in comp.lang.c++, Christopher
Benson-Manica <at***@nospam.c yberspace.org> was alleged to have written:

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

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

[Jerry Coffin]

In article <bu**********@c hessie.cirr.com >, at***@nospam.cy berspace.org
says...

[ ... ]

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.

--
Later,
Jerry.

The universe is a figment of its own imagination.

[David Harmon]

On Mon, 19 Jan 2004 21:13:37 GMT in comp.lang.c++, Jerry Coffin
<jc*****@taeus. com> was alleged to have written:

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.

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]

David Harmon wrote:

On Mon, 19 Jan 2004 21:13:37 GMT in comp.lang.c++, Jerry Coffin
<jc*****@taeus. com> was alleged to have written:

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.

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.