Sprawdzanie czy lancuch string to palindrom - code review

Witam,

Mam napisać program który sprawdza czy wprowadzony łańcuch jest palindromem, przy czym program nie uwzględnia wielkości liter i spacji (tak ma być). Zastanawiam się, czy można to zrobić sprawniej i lepiej. Szczególnie interesuje mnie to co wpisałem jako komentarz w jednej z funkcji i czy dobrze użyłem funkcji bool.

 #include <iostream>
#include <string>
#include <cctype>
using namespace std;

string tostring (string str);
bool pallindrome (string str);

int main()
{
    string str;
    getline (cin, str);
    pallindrome (str) == 1? cout<<"TAK": cout<<"NIE";
}

string tostring (string w) // funkcja ktora zamienia wpisany lancuch na jedolitą wersje bez spacji i malych liter
{
    string str = w;
    for(int i = 0; i<str.size()-1; i++)  //rozumiem ze w takiej sytuacji konieczna jest iteracja. nie ma
        //funkcji ktora zamieni mi wzsytkie litery w lancuchu string na male bez iteracji (za: SO)
        str[i]=tolower(str[i]);

    int loc = str.find(' ');
    do{
    str.erase(loc, 1);
    loc = str.find(' ');
    }while (loc != string::npos);
    return str;

}


bool pallindrome (string lan)
{
    string str=(tostring(lan));
    int siz= str.size()-1;
    for (int i = 0, j = siz; i<siz/2; i++, j--)
    {
        if(str[i]!=str[j])
            return false;
    }
    return true;
}

Może po prostu to tutaj zostawię...

bool is_palindrome(string const &str) {
    return str == string{str.rbegin(), str.rend()};
}

• nie chcę niepokoić, ale tostring(string) -> string nie wygląda zbyt logicznie.
• jeżeli przekazujesz argument przez wartość, to nie musisz kopiować wartości ponownie - przekazanie przez wartość zapewniło Ci kopię.
• str.erase(std::remove(str.begin(), str.end(), ' '), str.end());
• nie, nie można zrobić czegoś dla każdego elementu jednocześnie nie przechodząc przez wszystkie elementy.

Nie wiem jak ma nie uwzględniać spacji - może po prostu trzeba je wyciąć przed sprawdzeniem?

Jeśli chodzi o resztę, to:

#include <iostream>
using namespace std;

bool is_palindrome(const std::string &str) {
	bool result = true;
	
	int i = 0;
	int j = str.size() - 1;
	
	while(i < j) {
		if (toupper(str[i]) != toupper(str[j])) {
			result = false;
			break;
		}
		++i;
		--j;
	}
	
	return result;
}

int main() {
    string str;
    
    while(getline (cin, str)) {
        cout << (is_palindrome(str) ? "TAK": "NIE") << "\n";
    }
    
    return 0;
}

http://ideone.com/ioZ6Pi

Prosiłeś o przegląd. W takim przypadku zawsze mogą rodzić się "święte wojny" o przekonania co dobre a co nie.
Z mojego doświadczenia...

#include <iostream>
#include <string>
#include <cctype>

using namespace std;

// Słaba nazwa. Lepiej np. only_letter_string, remove_spaces_... niż tostring. Twoja nazwa sugeruje że 
// dopiero po zadziałaniu argumenty stają się stringiem.
string tostring (string str);
// Raczej funkcję zwracającą bool, nazywaj is_palindrome lub isPalindrome. To 
// powszechna praktyka dla predykatów (true/false).
bool pallindrome (string str);
 
int main()
{
    // Trochę krótka nazwa. Na szczęsicie broni się przez bardzo niewielki zasięg jedynie 3 linii.
    string str;
    getline (cin, str);
    // W żadnym wypadku! Nie porównuj do 1'dynki! Albo nie porównuj wcale (bo zwraca 
    // true/false) albo porównaj do true (to ostatnie często jest wytykane jako "javowa" naleciałość) :-)
    //
    // Operator "?" także nie służy do wykonywania kodu (cout <<...). Abo porównaj albo wyświetl true.
    pallindrome (str) == 1? cout<<"TAK": cout<<"NIE";
}

// Tu raczej prześlij stałą referencję na string co będzie sygnałem że masz intencję go nie zmieniać a
// zwrócić nowy bez spacji i "śmieci"... 
string tostring (string w) // funkcja ktora zamienia wpisany lancuch na jedolitą wersje bez spacji i malych liter
{
    string str = w;

    // Jest taka funkcja :-) Ale to w drugim przykładzie.. 
    //
    // Tu porównujesz int do typu size_t. str.size() zwraca taki typ a nie int.
    for(int i = 0; i<str.size()-1; i++)  //rozumiem ze w takiej sytuacji konieczna jest iteracja. nie ma
        //funkcji ktora zamieni mi wzsytkie litery w lancuchu string na male bez iteracji (za: SO)
        str[i]=tolower(str[i]);

    // Khm... no dobra... a jak nie będzie żadnych spacji? Np. 1 słowo. Twój kod "się wywraca" :-/ 
    int loc = str.find(' ');
    do{
        // Tu masz nieprzeciętnie wiele realokacji danych. String po "wycięciu" znaku, przepisywany jest od
        // nowa bo jest w ciągłej pamięci.
        str.erase(loc, 1);
        // A co jak nie znajdzie? Brak sprawdzenia tego co ma być w loc.
        loc = str.find(' ');
    // Porównanie złych typów.
    }while (loc != string::npos);

    // Bardzo dobrze że kopia jest zwracana. Tu dobry kompilator tak naprawdę jej ... nie wykona :-)
    return str;
}

// Znów przesłał bym const string& . Powód to co wyżej.
// Bardzo zła nazwa lan. Tatalnie zła sugestia... "coś z sieci..... o to chodzi"? :-/
bool pallindrome (string lan)
{
    string str=(tostring(lan));
    // Lepiej nazwa char_count lub coś w tym stylu... 
    int siz= str.size()-1;
    // Pomysł dobry, wykonanie jednak może generować błędy jeśli pomylisz się w obliczeniach (popularne
    // < wersus <= itp.. 
    for (int i = 0, j = siz; i<siz/2; i++, j--)
    {
        if(str[i]!=str[j])
            return false;
    }
    return true;
}

Ja bym napisał jakoś tak...

 
#include <iostream>
#include <string>
#include <algorithm>
#include <cctype>

using namespace std;

string remove_puncs_and_spaces(const string& src_string);
bool is_palindrome (const string& src_string);
 
int main()
{
    string str;
    getline (cin, str);
    cout << str << " " << (is_palindrome(remove_puncs_and_spaces(str))
            ? "is" : "isn't") << " palindrome." << endl;
}

string remove_puncs_and_spaces (const string& src_string)
{
    string answer;
    answer.reserve(src_string.length());
    remove_copy_if(src_string.cbegin(), src_string.cend(), answer.begin(), [](char c) {
        return not (ispunct(c) or isspace(c));
    });
    transform(answer.begin(), answer.end(), answer.begin(), ::tolower);
    return answer;
}

bool is_palindrome (const string& src_string)
{
    return (src_string == string(src_string.rbegin(), src_string.rend()));
}

Wersja z ignorowaniem spacji i wielkości liter:

#include <iostream>
using namespace std;

bool is_palindrome(const std::string &str) {
	bool result = true;
	
	int i = 0;
	int j = str.size() - 1;
	
	while(i < j) {
		while(str[i] == ' ' && i < j) {
		  ++i;
		}

		while(str[j] == ' ' && i < j) {
		  --j;
		}
		
		if (toupper(str[i]) != toupper(str[j])) {
			result = false;
			break;
		}
		
		++i;
		--j;
	}
	
	return result;
}

int main() {
    string str;
    
    while(getline (cin, str)) {
        cout << (is_palindrome(str) ? "TAK": "NIE") << "\n";
    }
    
    return 0;
}

https://ideone.com/0izP0i

Smok pewnie by to zapisał krócej, ale póki co - jest jak jest.

Skoro spacje są pomijane, to może warto pomijać też znaki interpunkcyjne. Wtedy takie cudo
*"Taki cel ataku", dodał Kazik. "Erotomana na motorek - i zakład o dukata: leci, kat! * też będzie palindromem.
Chyba najprościej, to wpierw przepisać string pomijając nieistotne znaki, zamieniając jednocześnie wszystkie litery na małe. Kod w Javie:

    boolean isPalindrome(String str,boolean isCaseSensitive)
    {
        if(!isCaseSensitive)
        {
            str = str.toLowerCase();
        }
        String[] toSkip = {" ","!","-","_","+","=","{","}","[","]",";",":","\'","\"",",",".","?"};
        for(int i=0;i<toSkip.length;i++)
        {
            str = str.replace(toSkip],"");
        }
		StringBuilder sb = new StringBuilder(str);
		String reverse = sb.reverse().toString();
		return str.equals(reverse);
    }