Contents of string change when exiting loop

Question

I have a simple function, that checks if the strings given match a certain condition, then generate a 3rd string based on the 2 ones received as arguments. The 3rd string is fine but when I return it it suddenly turn into "\n".

string sReturn = "";
if (sText.size() != sPassword.size()) {
     //Checks to see if the texts match a condition
     return sReturn;
}
for (int foo = 0; foo < sText.size(); foo++) {
    sReturn = "";
    sReturn += (char)sText[foo] ^ (char)sPassword[foo];
}
return sReturn;

In the for sReturn is fine and has the right contents, but as soon as it exists the loop, the debugger suddenly tells me it's contents are "\n". What am I doing wrong ?

Answer 1

Why do you have sReturn = "" inside the loop. Shouldn't that be initialized before the loop?

In the given case sReturn will only ever have one character. In your case, I would assume the ^ operation is resulting in a \n character in the last iteration.

Answer 2

1. You don't have to initialize string with empty character array like:

   std::string sReturn = "";
   

   Default constructor meant to do it for you and is much more efficient. Correct code:

   std::string sReturn;
   

2. Assigning an empty string to sReturn on each iteration in your loop is incorrect. Not to mention that to clear a string you have to call std::string::clear ():

   sReturn = "";
   

   Correct code:

   sReturn.clear (); 
   

   But this should be removed from the loop at all in your case.

3. There is not need to explicitly convert the result of operator [] (size_t) into a character because it is a character:

   sReturn += (char)sText[foo] ^ (char)sPassword[foo];
   

   Correct code:

   sReturn += sText[foo] ^ sPassword[foo];
   

4. Using post-increment in for loop is not necessary. It makes an extra copy of the "foo" on every increment:

   for (int foo = 0; foo < sText.size(); foo++)
   

   This probably will be optimized by compiler but you have to get rid of this bad habit. Use pre-increment instead. Correct code:

   for (int foo = 0; foo < sText.size(); ++foo)
   

5. Calling std::string::size () on every iteration when string size does not change is not efficient:

   for (size_t foo = 0; foo < sText.size(); ++foo)
   

   Better code:

   for (size_t foo = 0, end_foo = sText.size(); foo < end_foo; ++foo)
   

   Note size_t type. You cannot store string size in 32-bit signed integer as it has not enough capacity to store large number. Correct type is size_t, which is returned by std::string::size () method.

Taking into account all above, the correct function should look something like this:

std::string
getMixedString (const std::string & text, const std::string & password)
{
    std::string result;
    if (text.length () != password.length ())
        return result;
    for (size_t pos = 0, npos = text.length (); pos < npos; ++pos)
        result += text[pos] ^ password[pos];
    return result;
}

But there is a problem if you want the final string to be human-readable. Using eXclusive OR (XOR) operator on two ASCII characters might or might not give you human readable character or even an ASCII character. So you may end up having resulting string that contains newlines, unreadable characters, some garbage whatsoever.

To solve this, you have to come up with some better algorithm to generate a string basing on two other strings. For example, you may use MD5 hash of both strings or encode them in base64.

Good luck!

Answer 3

You've already had the problem explained. I'll suggest a completely different way of doing things that I think eliminates most possibility of producing a similar mistake. First, I'd separate the "check if the texts match a condition" part from the "encode" part. Right now, you have one (fairly small) of code that seems to have two, mostly unrelated, responsibilities.

The encode part, I'd write something like this:

struct encode_byte { 
    char operator()(char a, char b) { 
        return a ^ b;
    }
};

std::transform(sText.begin(), sText.end(),
               sPassword.begin(), sPassword.end(),
               std::back_inserter(sResult),
               encode_byte());

Answer 4

string sReturn;
if (sText.size() != sPassword.size()) {
        return sReturn;
}
for (size_t foo = 0, end_foo = sText.size(); foo < end_foo; ++foo) {
        sReturn += sText[foo] ^ sPassword[foo];
}
return sReturn;

I have rewrote it now using everyone's tips. I appologize for the mistake caused by the lack of attention -- clearing the string everytime I run through the loop. I hope it is OK now, please tell me if there is anything that is wrong with it. Thanks to everyone for their prompt answers.