implementation dilema

[ma740988]

Consider the snippet:

# include <iostream>
# include <vector>

using namespace std;

class message {
std::string m_id_recipient;
int m_payload;
int m_count;
public:
explicit message (
const std::string& id_recipient, int payload, int count )
: m_id_recipient (id_recipient )
, m_payload(payload)
, m_count (count ) {}
std::string get_recipient_id() const { return m_id_recipient; }
int get_payload() const { return m_payload; }
int get_count() const { return m_count; }
};

class foo {
std::string str;
typedef std::vector<message> MSG_VEC;
MSG_VEC mv;
public:
foo(std::string st)
: str(st)
{ }
void add_msg(message& the_msg) { mv.push_back(the_msg); }
std::string get_foo_str() const { return str; }
};

class bar {
typedef std::vector<int> INT_VEC;
public:
explicit bar() {}
void do_a_special_thing( INT_VEC v )
{
}
};

class manager {
typedef std::vector<foo*> FOO_VEC;
FOO_VEC fv;
bar b;
public:
explicit manager(){}
~manager() {
FOO_VEC::iterator it;
for ( it = fv.begin(); it != fv.end(); ++it )
delete *it;
}
void add_foo(foo* ptr_foo)
{
fv.push_back(ptr_foo);
}
void add_foo_msg ( message& the_msg )
{
FOO_VEC::iterator it;
for ( it = fv.begin(); it != fv.end(); ++it )
{
const std::string& recipient = the_msg.get_recipient_id();
if ( strcmp(recipient.c_str(),
(*it)->get_foo_str().c_str()) )
{
(*it)->add_msg(the_msg);
break;
}
}
}

void do_foo_search( unsigned int seg_id )
{
FOO_VEC::iterator it;
for ( it = fv.begin(); it != fv.end(); ++it )
{
// find the segment_id
//call do_a_special_thing
}
}
};

int main()
{
manager mgr;
mgr.add_foo(new foo("foo1"));
mgr.add_foo(new foo("foo2"));

mgr.add_foo_msg( message("foo1", 0x4000, 1)); // i
mgr.add_foo_msg( message("foo1", 0x1000, 5));
mgr.add_foo_msg( message("foo2", 0x2000, 1)); // ii
mgr.add_foo_msg( message("foo3", 0x3000, 2));

}

Within the member function do_foo_seach, if I passed in the value 1.
I'd like store the values 0x4000 ( see i ) and 0x2000 (see ii ) in a
vector that I'll hand to the function do_a_special_thing.

What's an elegant way to achieve this? I'm messing around with an
approach here but it doesnt seem right.
Thanks in advance

[Victor Bazarov]

ma740988 wrote:

Consider the snippet:
[..]
void do_foo_search( unsigned int seg_id )
{
FOO_VEC::iterator it;

I probably don't understand your problem, to me this seems
quite obvious:

// define a vector here where you'll collect your 'ints'
for ( it = fv.begin(); it != fv.end(); ++it )
{
// find the segment_id
//call do_a_special_thing
if ((*it)->get_count() == seg_id) {
// store what you need in the vector of 'ints'
}
}
And when this loop ends, call your 'do_a_special_thing'
}
};

int main()
{
manager mgr;
mgr.add_foo(new foo("foo1"));
mgr.add_foo(new foo("foo2"));

mgr.add_foo_msg( message("foo1", 0x4000, 1)); // i
mgr.add_foo_msg( message("foo1", 0x1000, 5));
mgr.add_foo_msg( message("foo2", 0x2000, 1)); // ii
mgr.add_foo_msg( message("foo3", 0x3000, 2));

}

Within the member function do_foo_seach, if I passed in the value 1.
I'd like store the values 0x4000 ( see i ) and 0x2000 (see ii ) in a
vector that I'll hand to the function do_a_special_thing.

What's an elegant way to achieve this? I'm messing around with an
approach here but it doesnt seem right.

What doesn't seem right?

V

[Luke Meyers]

Egad, again. More comprehensible than the last one, but let's see what
we can do with it:

# include <iostream>
Are you using this anywhere?
# include <vector>
Where is <string>?
using namespace std;
Since you (properly) explicitly qualify all use of this namespace's
members, there's no need for this (evil) using-directive.
class message {
std::string m_id_recipient;
int m_payload;
int m_count;
public:
explicit message (
const std::string& id_recipient, int payload, int count )
What effect does the keyword "explicit" have in this context?
std::string get_recipient_id() const { return m_id_recipient; }
Why is it ordered id_recipient in two places, and recipient_id here?
In fact, what clarity is achieved by either? Maybe not just call it
"recipient?"
typedef std::vector<message> MSG_VEC;
Avoid all caps for anything but #define macros (which should themselves
be used as a last resort).
public:
foo(std::string st)
Should be string const& to avoid an unnecessary copy.
void add_msg(message& the_msg) { mv.push_back(the_msg); }
Should be message const&.
explicit bar() {}
What effect does the keyword "explicit" have in this context?
void do_a_special_thing( INT_VEC v )
Should pass by & or const&.
typedef std::vector<foo*> FOO_VEC;
Why are the foos stored as raw pointers? I bet sizeof(foo) is small
enough that you'd have no problem containing them directly. If you
must use pointers, use a smart pointer type (cf. e.g. std::auto_ptr,
boost::shared_ptr, boost::scoped_p) so that you don't have to hassle
yourself with inefficiently freeing up the memory yourself, and
possibly make a mistake (resulting in memory leaks or worse).
bar b;
"Barbie," geddit? ;)
FOO_VEC::iterator it;
for ( it = fv.begin(); it != fv.end(); ++it )
delete *it;
Better to call fv.end() just once, rather than each time through the
loop -- waste of good cycles. Better yet, when iterating over a
standard container, check to see if there's an STL algorithm that's a
good fit. Or, in this case, obviate the whole thing by heeding earlier
advice.
void add_foo(foo* ptr_foo)
Ugh, terrible variable name. Not only is it simply a description of
the type, it's expressed in the reverse order of how people typically
pronounce it. For example code, if there's no meaning intended, might
as well just call it "p."
void add_foo_msg ( message& the_msg )
const&.
const std::string& recipient = the_msg.get_recipient_id();
Binding a reference to a temporary is just asking for trouble, and
totally unnecessary here. You can just make it a const std::string, no
&.
if ( strcmp(recipient.c_str(),
(*it)->get_foo_str().c_str()) )
Um, std::string has operator==, you know...

Also, why on earth are you doing a linear search through a vector to
accomplish this? If you need to access values based on a string key,
use a std::map!
void do_foo_search( unsigned int seg_id )
Might want to provide a comment, especially when doing something so odd
as writing a "search" method that returns void.
Within the member function do_foo_seach, if I passed in the value 1.
I'd like store the values 0x4000 ( see i ) and 0x2000 (see ii ) in a
vector that I'll hand to the function do_a_special_thing.

Just to accomplish the problem as presented, I'd prefer to store the
data in a form more conveniently accessible by the intended means. The
way you've structured your data, there's pretty much no way to
accomplish the task besides a linear search through the whole set of
foos. After all, nobody outside of foo knows what messages a foo
instance holds, so you've left yourself no choice but to query each in
turn.

I'll take a crack at revising this code, will post again if I come up
with something worth showing.

I hope criticism is taken as intended (constructive).

Luke

[Luke Meyers]

Luke Meyers wrote:

I'll take a crack at revising this code, will post again if I come up
with something worth showing.

Okay, here's what I came up with. Seems to do what you described.

#include <vector>
#include <string>
#include <iostream>

class message {
std::string m_id_recipient;
int m_payload;
int m_count;
public:
message (std::string const& id_recipient,
int payload,
int count)
: m_id_recipient (id_recipient)
, m_payload(payload)
, m_count (count)
{ }

std::string get_recipient_id() const
{ return m_id_recipient; }

int get_payload() const
{ return m_payload; }

int get_count() const
{ return m_count; }
};
class foo {
typedef std::vector<message> msg_vec;

std::string name_;
msg_vec messages_;

public:
foo(std::string name)
: name_(name)
{ }

void add_msg(message& msg)
{ messages_.push_back(msg); }

std::string get_foo_str() const
{ return name_; }

typedef msg_vec::const_iterator const_iterator;

const_iterator begin() const {
return messages_.begin();
}

const_iterator end() const {
return messages_.end();
}
};

// bar removed (unused)

class manager {
typedef std::vector<foo> FOO_VEC;

FOO_VEC foos;

public:
void add_foo(foo const& f)
{
foos.push_back(f);
}

void add_foo_msg(message msg)
{
FOO_VEC::iterator end = foos.end();
for (FOO_VEC::iterator it = foos.begin(); it != end; ++it ) {
std::string recipient = msg.get_recipient_id();
if ( recipient == it->get_foo_str() )
{
it->add_msg(msg);
break;
}
}
}

void do_foo_search( unsigned int seg_id )
{
FOO_VEC::iterator end = foos.end();
for (FOO_VEC::const_iterator i = foos.begin(); i != end; ++i ) {
foo::const_iterator end = i->end();
std::vector<int> payloads;
for(foo::const_iterator j = i->begin(); j != end; ++j) {
if(j->get_count() == seg_id) payloads.push_back(j->get_payload());
}
if (!payloads.empty()) do_a_special_thing(payloads);
}
}

void do_a_special_thing(std::vector<int> payloads) {
std::cout << "Here's something special: ";
std::vector<int>::const_iterator end = payloads.end();
for(std::vector<int>::const_iterator it = payloads.begin(); it !=
end; ++it) {
std::cout << "0x" << std::hex << *it << " ";
}
std::cout << "\n";
}
};
int main()
{
manager mgr;
mgr.add_foo(foo("foo1"));
mgr.add_foo(foo("foo2"));
mgr.add_foo_msg( message("foo1", 0x4000, 1)); // i
mgr.add_foo_msg( message("foo1", 0x1000, 5));
mgr.add_foo_msg( message("foo2", 0x2000, 1)); // ii
mgr.add_foo_msg( message("foo3", 0x3000, 2));

mgr.do_foo_search(1);
}

HTH,
Luke

[Ali Çehreli]

"Luke Meyers" <n.***********@gmail.com> wrote in message
news:11*********************@g43g2000cwa.googlegro ups.com...

typedef std::vector<foo*> FOO_VEC;
Why are the foos stored as raw pointers? I bet sizeof(foo) is small
enough that you'd have no problem containing them directly. If you
must use pointers, use a smart pointer type

Good advice but...
(cf. e.g. std::auto_ptr,
std::auto_ptr cannot be used with standard containers.
boost::shared_ptr,
The only good option here...
boost::scoped_p[tr])
Cannot be copied or assigned; so not suitable for a standard container.
so that you don't have to hassle
yourself with inefficiently freeing up the memory yourself, and
possibly make a mistake (resulting in memory leaks or worse).

Again, good advice...

Ali

[Luke Meyers]

Ali Çehreli wrote:

Good advice but...

(cf. e.g. std::auto_ptr,

std::auto_ptr cannot be used with standard containers.

Yes; was giving examples of different kinds of smart pointers, not
saying they were all suitable in this case. Following the "teach a man
to fish" philosophy, figured if he was going to heed my advice he'd go
read up on those and learn which would work. This turned out to be
correct (in email).

boost::shared_ptr,

The only good option here...

Yup.

boost::scoped_p[tr])

Cannot be copied or assigned; so not suitable for a standard container.

Yup. But shared_ptr gets overused so much, I think it's worth
popularizing scoped_ptr a bit. ;)

I could have been more clear, thanks for helping out.

Luke