Comparing and assigning strings

Question

Would you please run through this code and provide your comments?

void quote_container::allocate(string *i_string, string *i_value){
 string temp_name = *i_string;
 double temp_value; 
 // Convert the input string to upper case. 
 stringToUpper(temp_name);
 if (temp_name =="OPEN"){
 temp_value = atof( (*i_value).c_str() );
 open = (float)temp_value;
 }
 else if (temp_name =="DAYHIGH") {
 temp_value = atof( (*i_value).c_str() );
 high = (float)temp_value;
 }
 else if (temp_name =="DAYLOW") {
 temp_value = atof( (*i_value).c_str() );
 low = (float)temp_value;
 }
 else if (temp_name =="LOW52") {
 temp_value = atof( (*i_value).c_str() );
 year_low = (float)temp_value;
 }
 else if (temp_name =="HIGH52") {
 temp_value = atof( (*i_value).c_str() );
 year_high = (float)temp_value;
 }
 else if (temp_name =="PRICEBANDLOWER") {
 temp_value = atof( (*i_value).c_str() );
 lower_price_band = (float)temp_value;
 }
 else if (temp_name =="PRICEBANDUPPER") {
 temp_value = atof( (*i_value).c_str() );
 upper_price_band = (float)temp_value;
 }
 else if (temp_name =="TOTALBUYQUANTITY"){
 temp_value = atof( (*i_value).c_str() );
 tbq = temp_value;
 }
 else if (temp_name =="TOTALSELLQUANTITY") {
 temp_value = atof( (*i_value).c_str() );
 tsq = temp_value;
 }
 else if (temp_name == "BUYQUANTITY5" || temp_name == "BUYQUANTITY4" || temp_name == "BUYQUANTITY3"|| 
 temp_name == "BUYQUANTITY2"|| temp_name == "BUYQUANTITY1") {
 temp_value = atof( (*i_value).c_str() );
 tbq2 = tbq2 + temp_value;
 }
 else if (temp_name == "SELLQUANTITY5" || temp_name == "SELLQUANTITY4" || temp_name == "SELLQUANTITY3"|| 
 temp_name == "SELLQUANTITY2" || temp_name == "SELLQUANTITY1") {
 temp_value = atof( (*i_value).c_str() );
 tsq2 = (float)tsq2 + temp_value;
 }
 else if (temp_name == "BUYPRICE1" || temp_name == "BUYPRICE2" || temp_name == "BUYPRICE3"|| 
 temp_name == "BUYPRICE4" || temp_name == "BUYPRICE5") {
 temp_value = atof( (*i_value).c_str() );
 avg_buy_price = avg_buy_price + (float)temp_value ;
 }
 else if (temp_name == "SELLPRICE5" || temp_name == "SELLPRICE4" || temp_name == "SELLPRICE3"|| 
 temp_name == "SELLPRICE2" || temp_name == "SELLPRICE1") {
 temp_value = atof( (*i_value).c_str() );
 avg_sell_price = tsq2 + (float)temp_value;
 }
 else if (temp_name == "FACEVALUE") {
 temp_value = atof( (*i_value).c_str() );
 face_value = (float)temp_value;
 }
 else if (temp_name == "PREVIOUSCLOSE") {
 temp_value = atof( (*i_value).c_str() );
 prev_close = (float)temp_value;
 }
 else if (temp_name == "LASTPRICE") {
 temp_value = atof( (*i_value).c_str() );
 last_price = (float)temp_value;
 }
 else if (temp_name == "TOTALTRADEDVOLUME") {
 temp_value = atof( (*i_value).c_str() );
 ttv = (float)temp_value;
 }
 else
 cout <<"\n\t Ignoring --"<<temp_name<<"---"<<*i_value;
}

Also

void quote_container::stringToUpper(string &s)
{
 for(unsigned int l = 0; l < s.length(); l++) 
 s[l] = toupper(s[l]); 
}

Could the above code be improved in any way?

• John Dibling: Recommendation #1: Don't use strings!
• Havenard: ^ Buffer lover, don't mind him.

Answer 1

Lets start with the simple one:

void quote_container::stringToUpper(string &s)
{
 for(unsigned int l = 0; l < s.length(); l++) 
 s[l] = toupper(s[l]); 
}

How about:

std::transform(s.begin(), e.end(), s.begin(), ::toupper);

Knowledge of the standard library is very useful.

Never pass anything as a pointer (this is C++ not C).

void quote_container::allocate(string *i_string, string *i_value){

I see no testing of i_string or i_value for NULL so they can't be NULL so there is no need to pass them as pointers. So you should be passing them as references. Since neither value is modified by the function you should also probably be passing them as const to prevent accidental modification of the original value.

void quote_container::allocate(string const& iString, string const& iValue){

Don't explicitly cast values.

open = (float)temp_value;
 // ^^^^^^^

This is code smell. If you must cast (and usually you should not) then use one of the C++ cast operators (there should never be a need to use a C cast operator (as above)). But in this case it looks like it is not needed. If you don't cast the same conversion will happen anyway.

In this case

avg_buy_price = avg_buy_price + (float)temp_value ;

You are getting worse results. Let the compiler to the cast for you. If you had let the compiler do the cast for you with:

avg_buy_price = avg_buy_price + temp_value;
// Then the compiler would have generated this code
avg_buy_price = static_cast<float>(static_cast<double>(avg_buy_price) + temp_value);

This way you would have retained a tiny bit more accuracy during the addition (which may have mattered). Before you cast it back to float.

As mentioned by "The Dr" (MrSmith) above factor out any common code. But I would do it differently. Put all your conditions in a map to make the code more readable. You actually have two types of assignment. The first is simple assignment while the second is accumulation. So we break these two types of action into their own maps

std::map<std::string, float*> assign= {
 {"OPEN", &open},
 {"DAYHIGH", &high},
 {"DAYLOW" &low},
 {"LOW52", &year_low},
 {"HIGH52", &year_high},
 {"PRICEBANDLOWER", &lower_price_band},
 {"PRICEBANDUPPER", &upper_price_band},
 {"TOTALBUYQUANTITY", &tbq},
 {"TOTALSELLQUANTITY", &tsq},
 {"FACEVALUE", &face_value},
 {"PREVIOUSCLOSE", &prev_close},
 {"LASTPRICE", &last_price},
 {"TOTALTRADEDVOLUME", &ttv}
 };
std::map<std::string, float*> accum = {
 {"BUYQUANTITY5", &tbq2},
 {"BUYQUANTITY4", &tbq2},
 {"BUYQUANTITY3", &tbq2},
 {"BUYQUANTITY2", &tbq2},
 {"BUYQUANTITY1", &tbq2},
 {"SELLQUANTITY5", &tsq2},
 {"SELLQUANTITY4", &tsq2},
 {"SELLQUANTITY3", &tsq2},
 {"SELLQUANTITY2", &tsq2},
 {"SELLQUANTITY1", &tsq2},
 {"BUYPRICE1", &avg_buy_price},
 {"BUYPRICE2", &avg_buy_price},
 {"BUYPRICE3", &avg_buy_price},
 {"BUYPRICE4", &avg_buy_price},
 {"BUYPRICE5" &avg_buy_price},
 {"SELLPRICE5", avg_sell_price}, /*avg_sell_price = tsq2 + (float)temp_value;*/
 {"SELLPRICE4", avg_sell_price}, /* I assumed this was a copy paste mistake you */
 {"SELLPRICE3", avg_sell_price}, /* made. If it is not then this array is slightly */
 {"SELLPRICE2", avg_sell_price}, /* More complex for SELL (but not much) */
 {"SELLPRICE1", avg_sell_price}
 };

Now that we have all the data stored in maps the code is highly simplified.

 std::map<std::string,float*>::const_iterator find;
 find = assign.find(iString);
 if (find != assign.end())
 {
 (*find->second) = atof( iValue.c_str() );
 }
 else
 {
 find = accum.find(iString);
 if (find != accum.end())
 {
 (*find->second) += atof( iValue.c_str() );
 }
 else
 {
 std::cout << "\n\t Ignoring --" << iString << "---" << iValue;
 }
 }

You can compreses the two maps into a single map (and thus reduce the code here) by using a functor or class abstraction as described by (@utnapistim) in his answer. Personally I would go with using std::function rather than building my own class hierarchy (but that's a you say potato I say tomato argument and down to personal preference).

• kris123456: Thanks for an eloberated answer. regarding your comment, i am receiving the content from source in the above format, hence using the same as is. Everytime appending it to sell & buy quantity.

Answer 2

Yes:

First, the if-else if-...- is big, difficult to maintain an monolithic.

Consider replacing it with a hierarchy of classes and allowing your function to do simple dispatching.

(also look for my comments in the code below):

class operation {
 vector<string>& patterns_;
protected:
 double &value_; // will be reference to value within quote_container
public:
 operation(const vector<string>& patterns, double& value)
 : patterns_(patterns), value_(value) {}
 virtual ~operation() = 0 {}
 virtual void apply(const string& i_value) = 0; // if you don't need
 // to check for null,
 // pass by reference
 virtual bool matches(string i_string) { // pass by value as we modify
 // this within the function
 transform(i_string.begin(), i_string.end(), i_string.begin(), toupper);
 return patterns_.end() != std::find(i_string);
 }
};
class assignment: public operation {
public:
 assignment(const vector<string>& patterns, double& value)
 : operation(patterns, value) {}
 virtual ~assignment() {}
 virtual void apply(const string& i_value) {
 value_ = atof(i_value.c_str() );
 }
};
class append: public operation {
public:
 append(const vector<string>& patterns, double& value)
 : operation(patterns, value) {}
 virtual ~append() {}
 virtual void apply(const string& i_value) {
 value_ += atof(i_value.c_str() );
 }
};

With this, your function becomes:

// defined in header (store by pointer to avoid slicing & for polymorphic behavior)
vector<unique_ptr<operation>> quote_container::operations;
quote_container::quote_container() {
 operations.push_back(new assignment{{"OPEN"}, open});
 operations.push_back(new assignment({"DAYHIGH"}, high});
 // ... other ops
 operations.push_back(new append(
 {"BUYQUANTITY5", "BUYQUANTITY4", "BUYQUANTITY3",
 "BUYQUANTITY2", "BUYQUANTITY1"}, tbq2});
 // ... other ops
}
quote_container::¬quote_container() {
 for_each(operations.begin(), operations.end(), default_delete<operation>);
}
void quote_container::allocate(string *i_string, string *i_value){
 for(operation* op: operations) {
 if(op->matches(*i_string)) {
 op->apply(*i_value);
 return;
 }
 }
 cout <<"\n\t Ignoring --"<<*i_string<<"---"<<*i_value;
}

Advantages:

• each part of the logic is now in a different place: you can define, test and maintain the operations separately (assignment, appending)

• this is very extensible: adding a new operation (for example, "weighted average with previous values") means defining a new class (which is also separately testable, maintainable and so on)

• modifying your dispatch map is different from the implementation of your operations

• all your functions became small (and more maintainable)

• quote_container::allocate doesn't change when you add a new operation (it is now boilerplate code, and will remain the same)

• Loki Astari: Rather tan a vector. You can use a map. This basically pushes your search loop down into the standard container itself.
• utnapistim: I thought about that, but a map would require indexing/comparison by key and disallow custom matching criteria. This was an easier implementation for explaining the new implementation.

Answer 3

First step: move the code common to all cases out of the conditions:
(you may add handling if *i_value cannot always be converted to float)

void quote_container::allocate(string *i_string, string *i_value){
 string temp_name = *i_string;
 double temp_value = atof( (*i_value).c_str() ); 
 stringToUpper(temp_name);
 if (temp_name =="OPEN"){
 open = (float)temp_value;
 }
 else if (temp_name =="DAYHIGH") {
 high = (float)temp_value;
 }
 else if (temp_name =="DAYLOW") {
 low = (float)temp_value;
 }
 else if (temp_name =="LOW52") {
 year_low = (float)temp_value;
 }
 else if (temp_name =="HIGH52") {
 year_high = (float)temp_value;
 }
 else if (temp_name =="PRICEBANDLOWER") {
 lower_price_band = (float)temp_value;
 }
 else if (temp_name =="PRICEBANDUPPER") {
 upper_price_band = (float)temp_value;
 }
 else if (temp_name =="TOTALBUYQUANTITY"){
 tbq = temp_value;
 }
 else if (temp_name =="TOTALSELLQUANTITY") {
 tsq = temp_value;
 }
 else if (temp_name == "BUYQUANTITY5" || temp_name == "BUYQUANTITY4" || temp_name == "BUYQUANTITY3"|| 
 temp_name == "BUYQUANTITY2"|| temp_name == "BUYQUANTITY1") {
 tbq2 = tbq2 + temp_value;
 }
 else if (temp_name == "SELLQUANTITY5" || temp_name == "SELLQUANTITY4" || temp_name == "SELLQUANTITY3"|| 
 temp_name == "SELLQUANTITY2" || temp_name == "SELLQUANTITY1") {
 tsq2 = (float)tsq2 + temp_value;
 }
 else if (temp_name == "BUYPRICE1" || temp_name == "BUYPRICE2" || temp_name == "BUYPRICE3"|| 
 temp_name == "BUYPRICE4" || temp_name == "BUYPRICE5") {
 avg_buy_price = avg_buy_price + (float)temp_value ;
 }
 else if (temp_name == "SELLPRICE5" || temp_name == "SELLPRICE4" || temp_name == "SELLPRICE3"|| 
 temp_name == "SELLPRICE2" || temp_name == "SELLPRICE1") {
 avg_sell_price = tsq2 + (float)temp_value;
 }
 else if (temp_name == "FACEVALUE") {
 face_value = (float)temp_value;
 }
 else if (temp_name == "PREVIOUSCLOSE") {
 prev_close = (float)temp_value;
 }
 else if (temp_name == "LASTPRICE") {
 last_price = (float)temp_value;
 }
 else if (temp_name == "TOTALTRADEDVOLUME") {
 ttv = (float)temp_value;
 }
 else
 cout <<"\n\t Ignoring --"<<temp_name<<"---"<<*i_value;
}