C++ ::toupper not allowing equality comparison?

Question

I am trying to convert a string to uppercase so I can manipulate it, but while I can successfully manipulate natural uppercase strings, as well as convert lowercase to uppercase, using this method of conversion fails to allow the manipulation.

For example, if I pass "hello" through the encryption, my encrypted string becomes "HELLO", but when I pass "HELLO" through (naturally capitalized), it correctly shifts.

Is there a different way of forcing uppercase that I need to be using or am I doing something wrong?

int Caesar::encrypt (const std::string &message, std::string &emessage) {
  int count = 0;
  emessage = message;
  std::transform(emessage.begin(), emessage.end(), emessage.begin(), ::toupper);
  for (std::string::size_type i = 0; i < message.size(); i++) {
    for (int j = 0; j < 26; j++) {
      if (emessage[i] == std_alphabet[j]) {
        std::replace(emessage.begin(), emessage.end(), message[i], c_alphabet[j]);
      }
    }
    count++;
  }
  return count;
}

constructor:

Caesar::Caesar (int shift) {
    // loop to populate vector with 26 letters of English alphabet
    // using ASCII uppcase letter codes
  for (int i = 0; i < 26; i++) {
    std_alphabet.push_back(i + 65);
  }
    // fills Caesar alphabet with standard generated alphabet
  c_alphabet = std_alphabet;
    // shifts Caesar alphabet based off the constructor parameter
  std::rotate(c_alphabet.begin(), c_alphabet.begin() + shift, c_alphabet.end());
}

test file:

void testCaesar() {
  Caesar test(4);
  std::string original = "HELLO";
  std::string encrypted = "";
  test.encrypt(original,encrypted);
  std::cout << encrypted << std::endl;
  std::cout << original << std::endl;
}

int main() {
  testCaesar();
  return 0;
}

Obviously there is a header and includes and stuff but that is the basic code

the header file includes the two private vectors

What is `std_alphabet`? Also your `replace` uses `message[i]`, did you mean `emessage[i]`? — Barry, Sep 03 '15 at 17:30
`std_alphabet` is a vector filled with the 26 English letters uppercase. `message[i]` is intentional because it gives the correct index value needed for the shift. — motifesta, Sep 03 '15 at 17:31
`std_alphabet` is a vector of characters, and `c_alphabet` is a vector of shifted characters. The `message&` reference is to the string to be shifted while `emessage&` is an empty string. Thus, I am manipulating the non const string... Confused as to why this isn't working though. — motifesta, Sep 03 '15 at 17:34
Can you provide a [minimal, **complete**, and **verifiable** example](http://www.stackoverflow.com/help/mcve)? — Barry, Sep 03 '15 at 17:39

score 2 · Accepted Answer · answered Sep 03 '15 at 18:23

The specific issue you are seeing is that you're replacing the wrong thing here:

std::replace(emessage.begin(), emessage.end(), message[i], c_alphabet[j]);

If message was lowercase, then emessage will be all upper-case letters - none of which will be message[i]. so that replacement won't do anything. You meant:

std::replace(emessage.begin(), emessage.end(), emessage[i], c_alphabet[j]);
                                               ^^^^^^^^^^^

That said, your algorithm is totally wrong as HELLO encrypts as BCBBA with a shift of 4. There is a 1-1 mapping on letters, so H and L cannot both go to B. What you want to do is shift each letter as you go by just replacing it with what its next letter should be. That is:

for (std::string::size_type i = 0; i < emessage.size(); ++i) {
    emessage[i] = c_alphabet[emessage[i] - 'A'];
}

With which you don't actually need the initial transformation step:

emessage = message;
for (std::string::size_type i = 0; i < emessage.size(); ++i) {
    emessage[i] = c_alphabet[::toupper(emessage[i]) - 'A'];
}

The whole thing can be abridged quite a bit by just dropping your count (which is just the size anyway, so is redundant) and taking the message by-value:

std::string encrypt(std::string from) { // intentionally copying
    for (char& c : from) {
        c = c_alphabet[::toupper(c) - 'A'];
    }
    return from;
}

Nice answer. Since OP didn't mention c++11, he may need to use something like this instead: `for (int i = 0; i < from.size(); i++) { from[i] = c_alphabet[::toupper(from[i]) - 'A']; }` — Paul H., Sep 03 '15 at 19:22

C++ ::toupper not allowing equality comparison?

1 Answers1