patterncppMinor
Cross-post from SO- Palindrome finding function
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
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
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:
You push the value str before checking it worked. It should be:
Or even better would be:
Why are you doing this:
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>>
Method 1: Use getline()
You seem to reversing the line just to print it:
Rather than reversing the array, just iterate it in reverse order:
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.
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.