Should one use std::string*, std::map* to avoid copying ?

[digz]

This is a very simplified version of something I am trying to
understand.
The State object holds the strings and maps , and I pass a reference
to State to the process Function which manipulates it based on the tag
passed in.

I cannot pass the strings/maps in question as references coz ..
a) thats redundant, state has everything, it increases number of
parameters unnecessarily
b) I have to do the case/switch outside the function to decide what to
pass in.( and this is a simple example )

Is it a good idea to use things like string* and std::map..* as i have
tried below

Thanks
Digz

------------------------
#include<string>
#include<map>

using namespace std;
typedef std::map<int, string> mapType;

struct State {
string pId_;
mapType pTypeNSymbol_;
string uId_;
mapType uTypeNSymbol_;
};

void process( State& state, int Tag){
typedef mapType::iterator mItr ;
string* prodId;
mapType* typeSymMap;

switch( Tag ) {
case 0:
prodId = &state.pId_;
typeSymMap = &state.pTypeNSymbol_;
break;
case 1:
prodId = &state.uId_;
typeSymMap = &state.uTypeNSymbol_;
break;
}
if (prodId->empty()) {
mItr it;
mItr end = typeSymMap->end();
if ( (it = typeSymMap->find(6)) != end){
*prodId = (*typeSymMap)[6];
}
}

if (!typeSymMap->empty()) {
mItr it = typeSymMap->begin();
*prodId = it->second;
}
}

 

[Salt_Peter]

This is a very simplified version of something I am trying to
understand.
The State object holds the strings and maps , and I pass a reference
to State to the process Function which manipulates it based on the tag
passed in.

State is not an object, its a type.

I cannot pass the strings/maps in question as references coz ..
a) thats redundant, state has everything, it increases number of
parameters unnecessarily
b) I have to do the case/switch outside the function to decide what to
pass in.( and this is a simple example )

Is it a good idea to use things like string* and std::map..* as i have
tried below

Nothing wrong with storing pointers. You are asking us if the code is
correct and we can't see how you are storing the pointers nor how you
are using the process function. Personally, i'ld have declared an
abstract State class with 2 derivatives, one for each type (instead of
confusing the setup with one variant). The more you try to confuse the
program by using tags to suggest an object is type A or type B, the
less the compiler can do its job: typechecking.

Its rather difficult to comment on anything else. Except to say that
the code looks rather confused to me.

Thanks
Digz

------------------------
#include<string>
#include<map>

using namespace std;
typedef std::map<int, string> mapType;

struct State {
string pId_;
mapType pTypeNSymbol_;
string uId_;
mapType uTypeNSymbol_;

};

void process( State& state, int Tag){
typedef mapType::iterator mItr ;
string* prodId;
mapType* typeSymMap;

switch( Tag ) {
case 0:
prodId = &state.pId_;
typeSymMap = &state.pTypeNSymbol_;
break;
case 1:
prodId = &state.uId_;
typeSymMap = &state.uTypeNSymbol_;
break;
}
if (prodId->empty()) {
mItr it;
mItr end = typeSymMap->end();
if ( (it = typeSymMap->find(6)) != end){
*prodId = (*typeSymMap)[6];
}
}

if (!typeSymMap->empty()) {
mItr it = typeSymMap->begin();
*prodId = it->second;
}

}

 

[Daniel T.]

This is a very simplified version of something I am trying to
understand.
The State object holds the strings and maps , and I pass a reference
to State to the process Function which manipulates it based on the tag
passed in.

I cannot pass the strings/maps in question as references coz ..
a) thats redundant, state has everything, it increases number of
parameters unnecessarily
b) I have to do the case/switch outside the function to decide what to
pass in.( and this is a simple example )

Is it a good idea to use things like string* and std::map..* as i have
tried below

I'd like to see how else this State object is used before commenting too
much. However the first thing I would do is break the function up a bit.

void processA( string& prodId, mapType& typeSymMap )
{
// why so short? See notes below...
if ( ! typeSyMap.empty() )
prodId = typeSymMap.begin()->second;
}

void process( State& state, int tag )
{
switch ( tag ) {
case 0:
processA( state.pId_, state.pTypeNSymbol_ );
case 1:
processA( state.uId_, state.uTypeNSymbol_ );
default:
assert( false && "Bad tag value" );
}
}

Still doesn't make much sense, but I don't know the problem domain.
Comments below:

#include<string>
#include<map>

using namespace std;
typedef std::map<int, string> mapType;

struct State {
string pId_;
mapType pTypeNSymbol_;
string uId_;
mapType uTypeNSymbol_;
};

Can you come up with some better names? Can you come up with some member
functions? The 'process' function below would be an ideal
member-function.

void process( State& state, int Tag){
typedef mapType::iterator mItr ;
string* prodId;
mapType* typeSymMap;

Initialize your variables at the point of definition.

switch( Tag ) {
case 0:
prodId = &state.pId_;
typeSymMap = &state.pTypeNSymbol_;
break;
case 1:
prodId = &state.uId_;
typeSymMap = &state.uTypeNSymbol_;
break;

What if Tag is neither 0 or 1? If it can only be 0 or 1, then why not a
bool?

}
if (prodId->empty()) {
mItr it;

Again, initialize your variables at the point of definition. However,
this variable "it" is completely pointless so remove it.

mItr end = typeSymMap->end();

Remove "end" as well. Extraneous variables cause headaches.

if ( (it = typeSymMap->find(6)) != end){
*prodId = (*typeSymMap)[6];
}
}

if (!typeSymMap->empty()) {
mItr it = typeSymMap->begin();

Another extraneous variable. At least this one is initialized...

*prodId = it->second;
}
}

For the two 'if' statements above... If typeSymMap.empty() == false,
then the final result will be *prodId == typeSymMap->begin()->second. If
typeSymMap.empty() == true then there won't be a key of '6' in it so the
first 'if' statement (where it tries to find a key of '6') is utterly
pointless.

 

[Howard]

This is a very simplified version of something I am trying to
understand.
The State object holds the strings and maps , and I pass a reference
to State to the process Function which manipulates it based on the tag
passed in.

I cannot pass the strings/maps in question as references coz ..
a) thats redundant, state has everything, it increases number of
parameters unnecessarily
b) I have to do the case/switch outside the function to decide what to
pass in.( and this is a simple example )

Is it a good idea to use things like string* and std::map..* as i have
tried below

Where do you "pass in" any strings/maps?

Also, what's "redundant" about a reference?

And what function are you doing the switch/case "outside of"?

One reason to use pointers in the way you have is because you have to
initialize a local reference at the point of declaration. You can't declare
the reference in one place but initialize it via two different paths (as in
a switch statement). Pointers, on the other hand, can be initialized
wherever you please. Using pointers like you have is fine, except that
there are probably better overall designs which would remove the need to use
pointers.

One idea, and probably the best, might be to use member functions and
polymorphism instead of switch statements.

Another might be to initialize local references (or pointers) via a call to
another function, thus avoiding the problem of trying to have two different
paths of initialization for a local reference. (Of course, that really just
pushes the problem out of the process function and into a different
function. But it clears things up locally, at least. )

-Howard