Comparison of strings shows not equal

Question

I'm learning C++, and one of the exercices (Accelerated c++) is to code a small program which finds palindromes.

To do this, I've created a vector holding a few words, and I'm then iterating over this vector to find palindromes. While iterating, I'm creating a copy of each word, reversing it, and then comparing if the reversed copy matches the origin word.

using namespace std;
vector<string> palindrome(vector<string> v){
  for(std::vector<string>::iterator iter = v.begin(); iter != v.end(); ++iter){
    string reversed_word;
    for(int i = iter->size(); i >= 0; i--){
      reversed_word += (*iter)[i];
    }
    if (!(*iter).compare(reversed_word)){
      cout << reversed_word << endl;
    }
    cout << reversed_word << endl;
    cout << *iter << endl;
  }
 return v
}


int main(){
  vector<string> dictionnary;
  dictionnary.push_back("coloc");
  dictionnary.push_back("elle");
  dictionnary.push_back("php");
  dictionnary.push_back("shahs");
  dictionnary.push_back("bonjour");
  dictionnary.push_back("random");

  palindrome(dictionnary);
  return 0;
}

However, the condition !(*iter).compare(reversed_word) doens't return what's expected for words that are palindromes. To check, I've displayed the words and the reverse words, and they do match.

What am I missing here ?

Answer 1

How many times does this loop iterate?

for(int i = (*iter).size(); i >= 0; i--)

You probably want size()-1 so it won't look past the end of the word.

Likewise, iter <= v.end() is seldom right. end never points to anything; it is always invalid. The idiom is != v.end().

Also, if the function does not return a value, its return type should be void.

Answer 2

Code like this makes me sad! Even when corrected, it's still (pardon my being frank) ugly and hard to read.

The point of C++ and its standard library is to allow you to work at a relatively abstract level--but most of what you've written is at pretty close to the level of assembly language. Instead of using a vector or a string as an abstraction, you're dealing with individual characters, building up your reversed string one character at a time, and so on.

In terms of abstraction, this is barely baby step above assembly language (and with much uglier syntax than most assemblers use).

So let's sit back and consider what you're really doing.

Your taking a vector of inputs. You're processing each one, and producing a result. That's what std::transform does, so that's what you should probably use here.

std::vector<bool> results(v.size());

std::transform(v.begin(), v.end(), results.begin(), is_palindrome);

So that leaves us with the task of defining is_palindrome.

Here again, I'd advise trying to think at a higher level of abstraction. std::string has a constructor that takes a pair of iterators. std::string also supports reverse iterators--you can use rbegin() and rend() to get reverse iterators into a string.

Between those two things, we can create a reversed string in one fairly simple step:

std::string reversed(s.rbegin(), s.rend());

std::string also overloads most of the relevant operators, so code to compare two strings can be more readable: return s == reversed;

After that, I think it's worth mentioning one or two things that weren't possible when Accelerated C++ was written, but should be on any compiler that's even sort of close to current. The first is initialization lists. Instead of:

  vector<string> dictionnary;
  dictionnary.push_back("coloc");
  dictionnary.push_back("elle");
  dictionnary.push_back("php");
  dictionnary.push_back("shahs");
  dictionnary.push_back("bonjour");
  dictionnary.push_back("random");

...you can now write this like:

vector<string> dictionary{ "coloc", "elle", "php", "shahs", "bonjour", "random" };

Another is lambda expressions. Instead of defining is_palindrome as a function (or functor) we can specify an expression in-place:

std::transform(v.begin(), v.end(),
               results.begin(),
               [](std::string const &s) {
                   return s == std::string(s.rbegin(), s.rend());
               });

There's most of the program in only 5 lines of code--and no (*iter)[i] in sight anywhere.

Answer 3

Change iter <= v.end(); to iter != v.end();, and

for(int i = (*iter).size(); i >= 0; i--){

to

for(int i = (*iter).size() - 1; i >= 0; i--){

Read this : Why is "!=" used with iterators?

Answer 4

I believe you are adding an empty character in your first loop, when trying to get (*iter)[(*iter).size()].

If pos is equal to the string length and the string is const-qualified, the function returns a reference to a null character ('\0').

Can you try:

for(int i = (*iter).size()-1; i >= 0; i--){
      reversed_word = reversed_word + (*iter)[i];
    }

std::cout would not show this difference, even though the 2 strings are different.