Small code review

[Christopher Benson-Manica]

Assuming the input to this program is

1
2
3
0
2
3
4
0

, is its behavior guaranteed to be well-defined? Is there anything
else about it that warrants comments and/or ridicule? The idea is to
accept two lists of numbers (separated by 0's) and print the numbers
that are in exactly one of the two lists; it seems to work.

#include <iostream>
#include <set>

int main()
{
unsigned int foo;
std::set< unsigned int > actual;
std::set< unsigned int > etb;
std::cin >> foo;
while( foo ) {
actual.insert( foo );
std::cin >> foo;
}
std::cin >> foo;
while( foo ) {
etb.insert( foo );
std::cin >> foo;
}
for( std::set<unsign ed int>::iterator i=actual.begin( ) ; i !=
actual.end() ; ) {
std::set<unsign ed int>::iterator j=etb.find( *i );
if( j != etb.end() ) {
std::set<unsign ed int>::iterator del=i;
++i;
actual.erase( del );
etb.erase( j );
}
else {
++i;
}
}
for( std::set<unsign ed int>::iterator i=actual.begin( ) ; i !=
actual.end() ; i++ ) {
std::cout << "actual: " << *i << std::endl;
}
for( std::set<unsign ed int>::iterator i=etb.begin() ; i !=
etb.end() ; i++ ) {
std::cout << "etb: " << *i << std::endl;
}
return 0;
}

--
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.

[Victor Bazarov]

Re: Small code review

Christopher Benson-Manica wrote:[color=blue]
> Assuming the input to this program is
>
> 1
> 2
> 3
> 0
> 2
> 3
> 4
> 0
>
> , is its behavior guaranteed to be well-defined?[/color]

Methinks, yes.
[color=blue]
> Is there anything
> else about it that warrants comments and/or ridicule?[/color]

See inline with code. Nothing to ridicule. Just two small things
I'd do differently.
[color=blue]
> The idea is to
> accept two lists of numbers (separated by 0's) and print the numbers
> that are in exactly one of the two lists; it seems to work.
>
> #include <iostream>
> #include <set>
>
> int main()
> {
> unsigned int foo;
> std::set< unsigned int > actual;
> std::set< unsigned int > etb;
> std::cin >> foo;
> while( foo ) {
> actual.insert( foo );
> std::cin >> foo;
> }
> std::cin >> foo;
> while( foo ) {
> etb.insert( foo );
> std::cin >> foo;
> }
> for( std::set<unsign ed int>::iterator i=actual.begin( ) ; i !=
> actual.end() ; ) {
> std::set<unsign ed int>::iterator j=etb.find( *i );
> if( j != etb.end() ) {
> std::set<unsign ed int>::iterator del=i;
> ++i;[/color]

You could easily do it in one statement:

std::set<unsign ed int>::iterator del = i++;
[color=blue]
> actual.erase( del );
> etb.erase( j );
> }
> else {
> ++i;
> }[/color]

I personally don't like extraneous curly braces. Personally.
[color=blue]
> }
> for( std::set<unsign ed int>::iterator i=actual.begin( ) ; i !=
> actual.end() ; i++ ) {
> std::cout << "actual: " << *i << std::endl;
> }
> for( std::set<unsign ed int>::iterator i=etb.begin() ; i !=
> etb.end() ; i++ ) {
> std::cout << "etb: " << *i << std::endl;
> }
> return 0;
> }
>[/color]

V

[John Harrison]

Re: Small code review


"Christophe r Benson-Manica" <ataru@nospam.c yberspace.org> wrote in message
news:clbbgp$s16 $2@chessie.cirr .com...[color=blue]
> Assuming the input to this program is
>
> 1
> 2
> 3
> 0
> 2
> 3
> 4
> 0
>
> , is its behavior guaranteed to be well-defined? Is there anything
> else about it that warrants comments and/or ridicule? The idea is to
> accept two lists of numbers (separated by 0's) and print the numbers
> that are in exactly one of the two lists; it seems to work.
>
> #include <iostream>
> #include <set>
>
> int main()
> {
> unsigned int foo;
> std::set< unsigned int > actual;
> std::set< unsigned int > etb;
> std::cin >> foo;
> while( foo ) {
> actual.insert( foo );
> std::cin >> foo;
> }
> std::cin >> foo;
> while( foo ) {
> etb.insert( foo );
> std::cin >> foo;
> }
> for( std::set<unsign ed int>::iterator i=actual.begin( ) ; i !=
> actual.end() ; ) {
> std::set<unsign ed int>::iterator j=etb.find( *i );
> if( j != etb.end() ) {
> std::set<unsign ed int>::iterator del=i;
> ++i;
> actual.erase( del );
> etb.erase( j );
> }
> else {
> ++i;
> }
> }
> for( std::set<unsign ed int>::iterator i=actual.begin( ) ; i !=
> actual.end() ; i++ ) {
> std::cout << "actual: " << *i << std::endl;
> }
> for( std::set<unsign ed int>::iterator i=etb.begin() ; i !=
> etb.end() ; i++ ) {
> std::cout << "etb: " << *i << std::endl;
> }[/color]


This is one of the occasions when the STL really does work. The above three
loops could be written as a couple of lines

std::ostream_it erator<unsigned int> output(std::cou t, "\n");
std::set_symmet ric_difference( actual.begin( ), actual.end( ), etb.begin( ),
etb.end( ), output);

You would lose the output "actual: " and "etb: " but if you were worried
about that then try this

std::ostream_it erator<unsigned int> output(std::cou t, "\n");
std::cout << "actual: ";
std::set_differ ence(actual.beg in(), actual.end(), etb.begin(), etb.end(),
output);
std::cout << "etb: ";
std::set_differ ence(etb.begin( ), etb.end(), actual.begin(), actual.end(),
output);

That's pretty close to what you had.

john