Code review comments requested - C++ Language (Moderated)

C++Talk.NET
C++ language newsgroups

Archives

FAQ

Memberlist

Usergroups

Profile

Code review comments requested
Goto page 1, 2, 3, 4, 5, 6 Next

C++Talk.NET Forum Index -> C++ Language (Moderated)

View previous topic :: View next topic

Author

Message

John Potter
Guest

Posted: Sun Oct 16, 2005 9:39 am Post subject: Code review comments requested

This simple code is correct. Would it pass a code review in your shop? If not, how should it be rewritten? Any generalizations? void words(map<string, int> &m, ifstream &fin) { for(string s; fin >> s; ++m[s]) { ; } } TIA, John [ See http://www.gotw.ca/resources/clcm.htm for info about ] [ comp.lang.c++.moderated. First time posters: Do this! ]

Kai-Uwe Bux
Guest

Posted: Sun Oct 16, 2005 3:47 pm Post subject: Re: Code review comments requested

John Potter wrote:

Quote:

This simple code is correct. Would it pass a code review in your shop?
If not, how should it be rewritten? Any generalizations?

void words(map<string, int> &m, ifstream &fin) {
for(string s; fin >> s; ++m[s]) { ; }
}

For my *taste* that is just too much going on in one line. I would do:

void words ( std::map< std::string, unsigned long > & m,
ifstream & fin ) {
string s;
while ( fin >> s ) {
++ m[s];
}
}

so my changes are:

a) replace map by std::map. (minor, but helps me to know which map it is)
b) same thing for std::string
c) replace int by unsigned long (counting numbers are always positive,
also the standard makes better guarantees as to arithmetic with
unsigned types.)
d) replace the for loop by a while loop.

If I had reasons to be paranoid about overflows, I would do:

void words ( std::map< std::string, unsigned long > & m,
ifstream & fin ) {
string s;
while ( fin >> s ) {
// increment and detect overflow:
if ( 0 == ++ m[s] ) {
throw ( something );
}
}
}

Note that this only works nicely because int was replaced with an unsigned
type.

Best

Kai-Uwe Bux

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]

Alf P. Steinbach
Guest

Posted: Sun Oct 16, 2005 3:50 pm Post subject: Re: Code review comments requested

* John Potter:

Quote:

I'd

1) rename the function to 'getWordsFrom' or 'addWordsTo', and

2) in the case of 'getWordsFrom', add a 'words' wrapper function if this is
not just an internal function called from one place only, and

3) still in that case, make it swap, and

4) anyway change the 'ifstream' to just 'istream' -- there's no need to be
over-specific, is there? and

5) typedef the map<string, int> as e.g. 'WordCounts'.

In addition, if this isn't an implementation of a function declared earlier
with some comment, I'd add a little comment about what it requires and
guarantees.

Reasoning & code:

It's a command function, not a function that returns words, and the current
name doesn't tell which of 'getWordsFrom' or 'addWordsTo' it's meant to be.

In the case of 'getWordsFrom' I'd make it swap to emulate a 'wordsFrom'
function closest possible in the face of exceptions (just on principle, a good
convention, even though here the stream will probably be changed), with the
nice side-effects of possibly reducing memory usage in the client code and
possibly reducing scaffolding in the client code,

typedef map<string, int> WordCounts;

void getWordsFrom( istream& sin, WordCounts& wc )
{
WordCounts result;
for( string s; sin >> s; ++result[s] ) {}
result.swap( wc );
}

and if it's meant to be used like a library function instead of just a small
internal helper function called from one place only, I'd add the 'wordsFrom'
wrapper function, like so:

WordCounts wordsFrom( istream& sin )
{
WordCounts result;
getWordsFrom( sin, result );
return result;
}

to let the client code make an informed choice of which to use.

In the case of 'addWordsTo' I'd change the order of the arguments:

void addWordsTo( WordCounts& m, istream& sin )
{
for( string s; sin >> s; ++m[s] ) {}
}

for readability in the client code. This case is harder wrt. what to do when
an exception occurs. But since the only realistically possible exception
(exceptions are supposedly not turned on in the stream) is a bad_alloc, and
since that is usually pretty fatal anyway, I think it would be a waste to e.g.
copy the existing map and swap at the end just in order not to change things.

Generalizations, I don't know. In general Wink

it's not a good idea to
generalize just to realize two years later that man-days were wasted on
something never used, and perhaps get such a realization every few man-days.
But an obvious generalization would be to parameterize the concept of "word",
at least what should be regarded as whitespace -- and essentially I think
that means using something else than standard library streams, they're just so
awkward and inefficient and error-prone and complex and so on, including UB,
sort of like MFC, where like your example all is OK if you carefully restrict
yourself to the pre-chosen functionality and don't care much about efficiency
or portability or..., and anything else is overwhelmingly costly to do.

I'm sure someone will point out the "obvious" generalization of templatizing
on the character type.

I'd never do that for this function; it's a standard library stream there.

--
A: Because it messes up the order in which people normally read text.
Q: Why is it such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]

Pete Becker
Guest

Posted: Sun Oct 16, 2005 3:51 pm Post subject: Re: Code review comments requested

John Potter wrote:

Quote:

I would remove the semi-colon from the block controlled by the for
statement. It's rarely appropriate to add syntactic noise.

--

Pete Becker
Dinkumware, Ltd. (http://www.dinkumware.com)

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]

Niklas Matthies
Guest

Posted: Sun Oct 16, 2005 11:16 pm Post subject: Re: Code review comments requested

On 2005-10-16 09:39, John Potter wrote:

Quote:

Just two remarks:
1) The function is badly named. It should be something like "count_words".
2) I prefer "continue" for empty loop bodies.

-- Niklas Matthies

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]

Brian
Guest

Posted: Sun Oct 16, 2005 11:18 pm Post subject: Re: Code review comments requested

John Potter wrote:

Quote:

Of course it all depends on who the reviewers are and what are their
standards. I doubt any oraganization code standards would mention that
this is either the accepted practive or not. Definetely, as others have
mentioned the name of the function should be changed. Also, I'm not
really thrilled about the for loop. What's really going on is a
transformation of the data from the istream. Why not use
std::transform? Some might then argue, if you use std::transform, you
really don't need the function. But, in my mind the function name alone
will give the code maintainer a better grasp of what is going on than
just the name std::transform.

Brian

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]

Carlos Moreno
Guest

Posted: Sun Oct 16, 2005 11:19 pm Post subject: Re: Code review comments requested

John Potter wrote:

Quote:

I personally would not accept this code for any project that I'm
directing.

"Tricky/misleading code" would be my reason. The for loop is used
in a way that it's not the intended way -- though yes, I agree you're
iterating over elements from an input stream, but still; you're not
executing the loop "a given number of times", nor is it one of the
common exceptions to that rule (which I tend to oppose as well, but
at least those are commonly used -- e.g., for loops for C-style string
manipulation).

In your defense -- being just a one-liner, at least is not the breed
of "tricky code" that is next to impossible to understand; it's just
the "unconventional" breed of tricky code.

Anyway, not hard to guess: I'd rewrite it as:

string s;
while (fin >> s)
{
++m[s];
}

No, I would not accept it without the curly braces either :-)

Cheers,

Carlos
--

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]

Peter Dimov
Guest

Posted: Mon Oct 17, 2005 11:22 am Post subject: Re: Code review comments requested

John Potter wrote:

Quote:

Apart from the missing std:: qualifications, I don't see anything wrong
with the code.

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]

Carl Barron
Guest

Posted: Mon Oct 17, 2005 11:53 am Post subject: Re: Code review comments requested

Carlos Moreno <moreno_at_mochima_dot_com (AT) xx (DOT) xxx> wrote:

Quote:

John Potter wrote:
This simple code is correct. Would it pass a code review in your shop?
If not, how should it be rewritten? Any generalizations?

void words(map<string, int> &m, ifstream &fin) {
for(string s; fin >> s; ++m[s]) { ; }
}

I personally would not accept this code for any project that I'm
directing.

"Tricky/misleading code" would be my reason. The for loop is used
in a way that it's not the intended way -- though yes, I agree you're
iterating over elements from an input stream, but still; you're not
executing the loop "a given number of times", nor is it one of the
common exceptions to that rule (which I tend to oppose as well, but
at least those are commonly used -- e.g., for loops for C-style string
manipulation).

In your defense -- being just a one-liner, at least is not the breed
of "tricky code" that is next to impossible to understand; it's just
the "unconventional" breed of tricky code.

Anyway, not hard to guess: I'd rewrite it as:

string s;
while (fin >> s)
{
++m[s];
}

No, I would not accept it without the curly braces either :-)

to each his own but the bodies are probably going to compile to

the same identical code:)

if you want iteratation I am surprised that you did not suggest

void count_words(std::map<std::string> &out,std::istream &fin)
{
std::istream_iterator<std::string> begin(fin),end;

for(;begin!=end;++begin)
{
++m[*begin]
}
}

should be readable and obvious.

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]

Carl Barron
Guest

Posted: Mon Oct 17, 2005 11:53 am Post subject: Re: Code review comments requested

Brian <brm (AT) dls (DOT) net> wrote:

Quote:

John Potter wrote:
This simple code is correct. Would it pass a code review in your shop?
If not, how should it be rewritten? Any generalizations?

void words(map<string, int> &m, ifstream &fin) {
for(string s; fin >> s; ++m[s]) { ; }
}

Of course it all depends on who the reviewers are and what are their
standards. I doubt any oraganization code standards would mention that
this is either the accepted practive or not. Definetely, as others have
mentioned the name of the function should be changed. Also, I'm not
really thrilled about the for loop. What's really going on is a
transformation of the data from the istream. Why not use
std::transform? Some might then argue, if you use std::transform, you
really don't need the function. But, in my mind the function name alone
will give the code maintainer a better grasp of what is going on than
just the name std::transform.

Transform ?? I don't see it with transform, I do with for_eqch but
++m[str] is not an easy thing to do without a functor.

#ifndef COUNT_WORDS_HPP_
#define COUNT_WORDS_HPP_

#include <algorithm>
#include <iterator>
#include <map>
#include <string>
#include <istream>

class incr_map
{
std::map<std::string,int> &m;
public:
incr_map(std::map<std::string,int> &a):m(a){}
void operator () (const std::string &x)
{
++m[x];
};

inline void count_words
(
std::map<std::string,int> &m,
std::istream &fin
)
{
std::for_each
(
std::istream_iterator<std::string(fin),
std::istream_iterator incr_map(m)
);
}
#endif

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]

Yuri Khan
Guest

Posted: Mon Oct 17, 2005 11:53 am Post subject: Re: Code review comments requested

Kai-Uwe Bux wrote:

Quote:

void words ( std::map< std::string, unsigned long > & m,
ifstream & fin ) {
string s;
while ( fin >> s ) {
++ m[s];
}
}

so my changes are:

a) replace map by std::map. (minor, but helps me to know which map it is)
b) same thing for std::string

So, why are you explicitly qualifying std::string in the argument list
but not in the local variable declaration? Would you also qualify
std::ifstream for consistency?

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]

Dave Harris
Guest

Posted: Mon Oct 17, 2005 1:59 pm Post subject: Re: Code review comments requested

[email]jpotter (AT) lhup (DOT) edu[/email] (John Potter) wrote (abridged):

Quote:

It needs renaming, of course, but the most painful thing about it is the
abuse of "for". The third expression should advance through the sequence
we are iterating over, and "++m[s]" does not do that. It should be in the
loop body.

Also the first expression should really initialise the iteration, and
"string s" doesn't do that, either, but that is more forgivable. This:

for (string s; fin >> s; )
++m[s];

would be better, and

string s;
while (fin >> s)
++m[s];

is probably best. Keep the loop control part "fin >> s", which produces a
sequence of strings, separate to the loop body "++m[s]", which processes
it.

-- Dave Harris, Nottingham, UK.

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]

John Potter
Guest

Posted: Mon Oct 17, 2005 2:01 pm Post subject: Re: Code review comments requested

On 16 Oct 2005 19:18:24 -0400, Brian <brm (AT) dls (DOT) net> wrote:

Quote:

John Potter wrote:

void words(map<string, int> &m, ifstream &fin) {
for(string s; fin >> s; ++m[s]) { ; }
}

Also, I'm not
really thrilled about the for loop. What's really going on is a
transformation of the data from the istream. Why not use
std::transform?

I give up. How? I can see for_each.

struct Counter {
map<string, int>& m;
Counter (map<string, int>& m) : m(m) {
}
void operator() (string const& s) {
++ m[s];
}
};

for_each(istream_iterator<string>(fin),
istream_iterator<string>(), Counter(m));

Then we could reopen the argument whether that is better than

string s;
for (fin >> s; s; fin >> s)
++ m[s];

or

string s;
while (fin >> s)
++ m[s];

or the original. I can't see the transformation.

transform(istream_iterator<string>(fin),
istream_iterator<string>(), what_outputerator<?>(?),
dowhater(?));

I could see an output iterator initialized by map whose assignment
operator did the increment much like back_insert_iterator does a
push_back, but that would use copy not transform. The name of that
output iterator would need to tell all. Would
mapped_int_incriment_iterator do the job? How about

namespace std {
map<string, int> operator+ (map<string, int> m, string const& s) {
++ m[s];
return m;
}
} // std

m = accumulate(istream_iterator<string>(fin),
istream_iterator<string>(), m);

John

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]

kanze
Guest

Posted: Mon Oct 17, 2005 5:13 pm Post subject: Re: Code review comments requested

Peter Dimov wrote:

Quote:

Come now: the ifstream& (instead of istream&) is definitly bad
practice. And the name of the function certainly isn't very
invocative.

The problem is that a lot depends on house rules. For example,
most of the places I've been would prefer putting the iostream
parameter first, along the lines of getline. My own "house
rules" suggests that the elements in the for should concern loop
control, which certainly isn't the case here -- I'd definitely
use a while in this case.

I'd also want to see the specification of the function. I've
never seen a case where the above code would do something
useful. But since I don't know what the function is really
supposed to do... If the goal is to count occurances of each
word, then "the" and "The" should probably increment the same
entry. And you'd almost certainly want to exclude punctuation
as well. And what about the possibility of ++ overflowing;
you'd certainly want to detect the case, although typically,
aborting with an error message might be acceptable handling.
But without knowing the exact specifications...

--
James Kanze GABI Software
Conseils en informatique orientée objet/
Beratung in objektorientierter Datenverarbeitung
9 place Sémard, 78210 St.-Cyr-l'École, France, +33 (0)1 30 23 00 34

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]

kanze
Guest

Posted: Tue Oct 18, 2005 10:20 am Post subject: Re: Code review comments requested

John Potter wrote:

Quote:

On 16 Oct 2005 19:18:24 -0400, Brian <brm (AT) dls (DOT) net> wrote:

John Potter wrote:

void words(map<string, int> &m, ifstream &fin) {
for(string s; fin >> s; ++m[s]) { ; }
}

Also, I'm not really thrilled about the for loop. What's
really going on is a transformation of the data from the
istream. Why not use std::transform?

I give up. How? I can see for_each.

struct Counter {
map<string, int>& m;
Counter (map<string, int>& m) : m(m) {
}
void operator() (string const& s) {
++ m[s];
}
};

for_each(istream_iterator<string>(fin),
istream_iterator<string>(), Counter(m));

Then we could reopen the argument whether that is better than

string s;
for (fin >> s; s; fin >> s)
++ m[s];

Just curious, but does this even compile? I always thought that
the type of the middle element of a for had to be convertible to
bool, and that string isn't.

Or did you mean:

for ( fin >> s ; fin ; fin >> s ) {
++ m[ s ] ;
}

Quote:

or

string s;
while (fin >> s)
++ m[s];

or the original. I can't see the transformation.

transform(istream_iterator<string>(fin),
istream_iterator<string>(), what_outputerator<?>(?),
dowhater(?));

I could see an output iterator initialized by map whose
assignment operator did the increment much like
back_insert_iterator does a push_back, but that would use copy
not transform.

Rather a bit of iterator abuse, no? I willingly use filtering
iterators or even iterators, but somehow or other, the results
of the iteration must correspond to a sequence.

Quote:

The name of that output iterator would need to tell all. Would
mapped_int_incriment_iterator do the job? How about

namespace std {
map<string, int> operator+ (map<string, int> m, string const& s) {
++ m[s];
return m;
}
} // std

m = accumulate(istream_iterator<string>(fin),
istream_iterator<string>(), m);

I like the idea of using accumulate, but I think I'd go with
something more like:

class CountingAccumulator
{
public:
CountingAccumulator( std::map< std::string, int >& table )
: myTable( &table )
{
}

CountingAccumulator operator+(
std::string const& rhs ) const
{
++ (*myTable)[ rhs ] ;
return *this ;
}
private:
std::map< std::string, int >*
myTable ;
} ;

:-). Unline your solution, it is perfectly legal (since it
doesn't add anything to std: Smile

. Probably more important in
practice, it's going to be orders of magnitude faster. (I
presume you forgot the smiley in yor suggestion: I know that
premature optimization is bad, but two deep copies of the map
for each word is probably going too far in the other direction.)

While we're at it, however: can anyone tell me why accumulate
uses accu = accu + elem/accu = binop( accu, elem ), instead of
the more natural accu += elem/binop( accu, elem )? Using
accumulate to calculate things like CRC or MD5 seems very
natural, but the performance implications can't be ignored. For
the moment, I've ensured that all of the data members are PODs
(although std::vector would have been nice in MD5), and let the
compiler generate the copy constructor. In the case of CRC,
everything is inlined, so the compiler should be able to get rid
of the copies completely. In the case of MD5, I'm hoping that
the copying will not be too noticeable compared to the amount of
work being done for each character, but there are 144 bytes of
state, so I don't know; I may end up having to split it into
two, a real accumulator, and an accumulator reference, something
like the CountingAccumulator above. In both cases, an
std::accumulate which used += would perform significantly
better.

In real life, I had to forgo using std::accumulate for the MD5,
as it was too slow. The interface is still there, but there is
also an append function, which takes two iterators. In the case
of CRC, I was only using it on fairly small blocks, and it was
immediately followed by a synchronized write, so performance
wasn't such an issue. Which is nice, because something like:

fout << data
<< Gabi::HexFmt( 8 )
<< std::accumulate( data.begin(), data.end(), CRC32()
).value() ;

is actually pretty cool, I think.

--
James Kanze GABI Software
Conseils en informatique orientée objet/
Beratung in objektorientierter Datenverarbeitung
9 place Sémard, 78210 St.-Cyr-l'École, France, +33 (0)1 30 23 00 34

[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]

Display posts from previous:

	C++Talk.NET Forum Index -> C++ Language (Moderated)	All times are GMT Goto page 1, 2, 3, 4, 5, 6 Next
Page 1 of 6

You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum