HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Cross-post from SO- Palindrome finding function

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
crossfunctionpostpalindromefindingfrom

Problem

I didn't know this site existed before now... awesome!

I just made a thread here: https://stackoverflow.com/questions/8511620/c-palindrome-finder-optimization

#include 
#include 
#include 
#include 
#include 
using namespace std;

bool isPal(string);

int main()
{
    vector sVec;
    vector sWords;
    vector sTwoWords1;
    vector sTwoWords2;
    char myfile[256]="/home/Damien/Test.txt";
    ifstream fin;
    string str;
    fin.open(myfile);
    if(!fin){
        cout > str;
        sWords.push_back(str);
        if(!fin){
            break;
        }
        if(isPal(str)){
            sVec.push_back(str);
        }
        else{
            getline(fin, str);
        }
    }
    reverse(sVec.begin(), sVec.end());
    for(int i =0; i < sVec.size(); i++){ 
        cout << sVec[i] << " is a Palindrome " <<endl;
    } 

    // Test 2 
    for(int i=0; i<sWords.size(); i++)
    {                                         
        for(int j=(i+1); j<sWords.size(); j++){ 
            str = sWords[i]+sWords[j];
            if(isPal(str)){
                sTwoWords1.push_back(sWords[i]);
                sTwoWords2.push_back(sWords[j]);
            }
        }
    }
    fin.close();
    for(int i=0; i<sTwoWords1.size();i++){ 
        cout << sTwoWords1[i] << " and " << sTwoWords2[i] << " are palindromic. \n";
    } 
    return 0;
}
bool isPal(string& testing) {
    return std::equal(testing.begin(), testing.begin() + testing.size() / 2, testing.rbegin());
}


About optimizing some code, assuming that I take the revision to my isPal function, what else can I do to optimize the hell out of my code?

Thanks a lot for any / all recommendations!

Wordfile: http://www.filedropper.com/ospd3

Solution

Reading a file:

while(fin){ 

    fin >> str; 
    sWords.push_back(str); 
    if(!fin){ 
        break; 
    }


You push the value str before checking it worked. It should be:

while(fin)
{ 
    fin >> str; 
    if(!fin){ 
        break; 
    } 
    sWords.push_back(str);


Or even better would be:

while(fin >> str)
{    
    sWords.push_back(str);


Why are you doing this:

else{
        getline(fin, str);
    }


Mixing operator >> and getline() makes the code hard to read.

Either use all operator >> or use getline() to get a line at a time then parse the line internally.

Note:: operator>> will ignore the '\n' at the end of the line and work. So if your file is a list of words one per line there is no need to use getline(). If on the other hand your file is a lot of text and you only want the first word on each line then you need to use getline().

Method 1: Use operator>>

// If your input file has one word per line (or you want to use all words)
 while(file >> word)
 {
      // Do stuff
 }


Method 1: Use getline()

// If your input file contains lots of words
 // But you only want to use the first word on each line
 while(std::getline(file, line))
 {
     std::stringstream  linestream(line);
     linestream >> word;

     // Do stuff
 }


You seem to reversing the line just to print it:

reverse(sVec.begin(), sVec.end());                          
for(int i =0; i < sVec.size(); i++){                                
    cout << sVec[i] << " is a Palindrome " <<endl;                                  
}


Rather than reversing the array, just iterate it in reverse order:

for(std::vector::const_iterator& loop = sVec.rbegin(); loop != sVec.rend(); ++ loop)
{                               
    cout << (*loop) << " is a Palindrome " <<endl;                                  
}


Note on efficiency:

Using push_back() on an vector can be in-effecient if you are pushing a lot of data. As evertime the vector runs out of space it must allocate more memory and copy the string to the new buffer. You help by pre-allocting space.

sVec.reserve(1000); // Guess at the size
                    // Note when the vector runs out of space it will approximately
                    // double its internal buffer.

Code Snippets

while(fin){ 

    fin >> str; 
    sWords.push_back(str); 
    if(!fin){ 
        break; 
    }
while(fin)
{ 
    fin >> str; 
    if(!fin){ 
        break; 
    } 
    sWords.push_back(str);
while(fin >> str)
{    
    sWords.push_back(str);
else{
        getline(fin, str);
    }
// If your input file has one word per line (or you want to use all words)
 while(file >> word)
 {
      // Do stuff
 }

Context

StackExchange Code Review Q#6849, answer score: 4

Revisions (0)

No revisions yet.