Re: Should one use std::string*, std::map* to avoid copying ?
digz <Digvijoy.C@gmail.com> wrote:
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.