palindrome

[jimJohnson]

I need to take my last C++ class over one more time and just trying to get a head start on the programs we had been assigned. I will provide the instructions and was wanting to get any feedback on what I should get rid of my current program and will probably look for additional help picking apart this program. The first thing I was wanting to know is if memory-mapped file input/output is set up correctly. Here are the instructions...

Create a utility that transfers words that are palindromes in the input file into the output file. Place a space between each palindrome written to the output file.
The input path and file name and the output path and file name must be gathered at runtime from the user. This can be accomplished via command line inputs (see argv and argc parameters in the main() function).
The program must use memory-mapped file input/output.
The contents of the input file must not be changed by your program (read only).
The definition of a palindrome, for purposes of this programming assignment, is a word which reads the same backward or forward. Examples of palindromes are mom, pop, kayak, noon, radar, and racecar.
While there are a number of methods available to test to see if a word is a palindrome, the most common method is to use a stack. Character[0] of the original string can be compared with character[n-1]. The next comparison would address character[1] with character[n-2]. Comparison continues until all characters are compared (the stack is empty) or there is an inequality (the word is not a palindrome).
The output file should contain only those words that are palindromes from the input file. Each word in the output file should be separated by a space.
Since the length of the output file is determined at runtime, and there is no way to predict how many palindromes will be present in the input file (without actually performing the test), there is no means to accurately predict the output file size required. Set the output file size to the input file size.

Here is my code...

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

//FUNCTIONS
bool isPalindrome(string);

//begin int main
int main(int argc, char * argv[])
{
	//Declare variables
	ifstream in;
	ofstream out;
	bool palin;
	string data;

	//open the file
	in.open(argv[1]);

	//Statement if the file does not open
	if(in.fail())          
       { 
		   //Cout statement to show the error
			cout<<"File not opening!" << endl;

			//Make a pause
			system("pause");
        
			//Return a value
			return 1;
        }

	//Open the file
	out.open(argv[2]);
	in>>data;

	//Perform while statement
	while(in)
    {
		palin=isPalindrome(data);
		if(palin)

        out<<data<<" ";
		in>>data;
    }

	//Close the in
	in.close();

	//Close the out
	out.close();
}

//Function to check Palindrome
bool isPalindrome(string input)
{
	//Declare variables
	int i=0,j;
	bool y=false;

	j=input.length()-1;

	//While statment to check i and j
	while(input[i]==input[j]&&i<=j)
    {
		i++;
        j--;
    }

	//If statement to check if i > j
	if(i>j)

	//Show that y is true
    y=true;

	//Return y
	return y;
}

[arkoenig]

Memory-mapped IO is not part of standard C++, which means that your instructor must be expecting you to use some locally defined facilities for the purpose. Your first job is to find out what those facilities are.

While you're doing that, I have three other suggestions:

1) Most of your comments are so obvious that they add nothing to the code. For example:

// If statement to check if i > j
if (i>j)

// Show that y is true
y=true;

// Return y
return y;

These comments make your program bigger, and therefore increase the amount of time it takes to read it; but they do not make the program any easier to understand.

2) Learn how to indent C++ code. Line 73, which is the beginning of an if statement, is indented less than line 76, which is the rest of that if statement. You should never write code like that.

3) Don't define useless variables. For example, your variables y and palin are each used in only a single place: You assign a value to the variable, use it once, and then never use it again. So, for example, instead of writing

y=true;
return y;

you can simply write

return true;

and instead of writing

palin=isPalindrome(data);
if(palin) // ...

you can write

if (isPalindrome(data)) // ...

In both cases you can not only get rid of the statement that assigns to the variable, but you can also get rid of the variable's declaration.

I note in passing that your isPalindrome function fails if you give it an empty string as input. I will leave that problem to you to fix.

[jimJohnson]

I did make some changes to my program and besides the memory mapped part which I am currently researching on can someone tell me from the instructions provided that everything else is correct?

#include<iostream>
#include <fstream>
#include<string>
#include <algorithm>
using namespace std;

//FUNCTIONS
bool isPalindrome(string);

//begin int main
int main(int argc, char * argv[])
{
	//Declare variables
	ifstream in;
	ofstream out;
	bool palin;
	string data;

	//open the file
	in.open(argv[1]);

	//Statement if the file does not open
	if(in.fail())          
       { 
		   //Cout statement to show the error
			cout<<"File not opening!" << endl;

			//Make a pause
			system("pause");
        
			//Return a value
			return 1;
        }

	//Open the file
	out.open(argv[2]);
	in>>data;

	//Perform while statement
	while(in)
    {
		palin=isPalindrome(data);
		if(palin)

        out<<data<<" ";
		in>>data;
    }

	//Close the in
	in.close();

	//Close the out
	out.close();
}

//Function to check Palindrome
bool isPalindrome(const string& input)
{
	// copy original string
	string sTemp = input;

	// reverse the original string
    std::reverse(sTemp.begin(), sTemp.end());

    // if reversed is same as original, return true.
    // else return false.
	return (input == sTemp);  
}

[arkoenig]

In my earlier remarks I made a number of suggestions, including:

1) Most of your comments are so obvious that they add nothing to the code.

2) Learn how to indent C++ code.

As far as I can tell, you have ignored these remarks. Why should I look for new problems if you haven't done anything about the ones I already found?

[Narue]

2) Learn how to indent C++ code.

The code is indented properly. Perhaps you could get to the root of the problem instead and mention that mixing tabs and spaces can affect indentation when pasted outside of an IDE.

[arkoenig]

The code is indented properly. Perhaps you could get to the root of the problem instead and mention that mixing tabs and spaces can affect indentation when pasted outside of an IDE.

Aha! Frankly, the thought had not occurred to me that the website (or, perhaps, the combination of the website and my browser) would display mixed-tabs-and-spaces code so confusingly.

Looking at the code again, I am guessing that it was written in an IDE that assumes 4-character tabs, and displayed by an engine that assumes 8-character tabs.

I stand by my remark about trivially redundant comments.

[jimJohnson]

O its kool man just had me confused as I was trying to figure out what was wrong with the spacing. As for the comments I will do my best to improve them but it was pretty much what I used from the example I got from the instructor the last year I took the course. I guess really where am at this point in my program is for someone to take a glance over my program and from the instructions I provided what I have right and what I have incomplete or wrong if that makes sense? If it does not I apologize and will try to explain it better.

[WaltP]

Change your IDE/Editor to convert TABs to SPACEs. That will solve the problem...

[Narue]

>bool isPalindrome(string);
I'm of the opinion that parameter names are important even in your prototypes. Also note that the type should be exact. Presently you have a type mismatch because the declaration claims a string object and the definition claims a reference to a const string object. Those are different types.

>in.open(argv[1]);
This can be done with the constructor, and in general it's better to initialize at the point of definition as well as define your variables shortly before they're used. This tends to make code easier to read as you don't have a slew of variables at the top of the function and the need to guess where they're used.

argv may not be large enough as well, you should check the value of argc to verify that everything is kosher for opening the file.

>if(in.fail())
Not wrong, per se, but it's more conventional to say if (!in) .

>cout<<"File not opening!" << endl;
cerr should be preferred for error messages. You can also call perror (in <cstdio>) to display a friendly string representing the error that actually occurred. It's not strictly portable, but I have yet to see it not work. ;)

>system("pause");
This isn't a portable construct, and it's potentially a security risk. The pause program may not exist (which is why it's not portable), and the pause program could be added or replaced with a malicious program. Use the system function with great care, or not at all.

>return 1;
Only three return values are portable: 0, EXIT_SUCCESS, and EXIT_FAILURE. The latter two are macros defined in <cstdlib>.

in>>data;
while (in)
{
    ...
    in>>data;
}

This is an awkward construct. You can simply move in>>data to the loop condition and rely on the fact that the >> operator returns a reference to the stream object.

>palin=isPalindrome(data);
There's no need for the palin variable in this case, as you can call isPalindrome directly from the if statement's condition.

>in.close();
>out.close();
These are both unnecessary. When an ifstream or ofstream destructor is called, any open file is closed automatically. You can simply open the stream and forget about it unless the object has a longer lifetime than you want the file open.

#include <iostream>
#include <fstream>
#include <string>
#include <algorithm>
#include <cstdio>
#include <cstdlib>
using namespace std;

bool isPalindrome(const string& input);

int main(int argc, char *argv[])
{
    if (argc > 2) {
        ifstream in(argv[1]);

        if (!in) { 
            perror("Error opening file");
            return EXIT_FAILURE;
        }

        ofstream out(argv[2]);
        string data;

        while (in>> data) {
            if (isPalindrome(data))
                out<< data <<' ';
        }
    }
}

bool isPalindrome(const string& input)
{
    string sTemp = input;

    reverse(sTemp.begin(), sTemp.end());

    return (input == sTemp);
}

[jimJohnson]

Sorry for not getting back to anyone in a while with this program but I have rated. I have made the changes that some of you have asked and have just a few issues yet I believe. Without giving me the code Can someone kind of explain to me what I still need to do from the instructions I gave and\or tell me what I have completed and I can try to figure the rest out. Thanks in advance and will rate favorably...

#include <iostream>
#include <cstdio>
#include <algorithm>
#include <string>
#include <cstdlib>
#include <fstream>

using namespace std;

bool isPalindrome(const string& input);

int main(int argc, char *argv[])
{
	if (argc > 2) 
	{
		ifstream in(argv[1]);

		if (!in) 
		{
			perror("Error opening file");
			return EXIT_FAILURE;
		}

	
		ofstream out(argv[2]);
		string data;

		while (in>> data) 
		{
			if (isPalindrome(data))
			out<< data <<' ';
		}
	}
}


bool isPalindrome(const string& input)
{
	string sTemp = input;
	reverse(sTemp.begin(), sTemp.end());

	return (input == sTemp);

}

[vijayan121]

You can check if a string is a palindrome without having to make a copy; just iterate from either end looking for a mismatch.

bool is_palindrome( const std::string& str )
{
    std::string::const_iterator mid = str.begin() + str.size() / 2 ;
    return !str.empty() &&
        std::mismatch( str.begin(), mid, str.rbegin() ).first == mid ;
}

And to be pedantically correct, you need a return statement after the while loop: return in.eof() && !out ? 0 : EXIT_FAILURE ;

[jimJohnson]

Made a ton of changes and was seeing if someone could take a look at code and let me know if there is anything they would change. Hoping that the depth of my code shows I put in alot of time and not just looking for answers...

#include <windows.h>
#include <iostream>
#include <stack>
#include <queue> 
using namespace std;




int main(int argc, char *argv[])
{
	HANDLE inFile;
    HANDLE inFileMap;
    PVOID pvInFile;
    DWORD dwInFileSize;
    int i = 0;
	
	


	//Start Inputfile Mapping

	cout<<"Enter Input path and filename: ";
//	cin>>argc;
	cout<<endl;
	//Test Files -------------------------------------------------------------
			 char inFilename[] = "c:\\InTest.txt";
  
			 argv[1] = "c:\\InTest.txt";
  //--------------------------------------------------------------------------

      //returns a HANDLE to the open file.
	inFile = CreateFile(argv[1],
                        GENERIC_READ,
                        0,
                        NULL,
                        OPEN_EXISTING,
                        FILE_ATTRIBUTE_NORMAL,
                        NULL);

      if (inFile == INVALID_HANDLE_VALUE)
      {
            cout << "inFile could not be opened.";
            return(FALSE);
      }
      
      //creates or opens a named or unnamed file mapping object
      //for file created.
      inFileMap = CreateFileMapping(inFile,
                                    NULL,
                                    PAGE_READONLY,
                                    0,
                                    0,
                                    NULL);
      if (inFileMap == NULL)
      {
            cout << "inFile map could not be opened";
            CloseHandle(inFile);
            return(FALSE);
      }
      
      //reserves address space for the file's data
            pvInFile = MapViewOfFile(inFileMap,
                               FILE_MAP_READ,
                               0,
                               0,
                               0);
      if (pvInFile == NULL)
      {
            cout << "Could not map view of pvInFile";
            CloseHandle(inFileMap);
            CloseHandle(inFile);
            return(FALSE);
      }

 
      //retrieves the size of the specified file in bytes.     
      dwInFileSize = GetFileSize(inFile, NULL);

      //cout << "dwInFileSize is: " << dwInFileSize << endl;

/*---------------------------------------------------------------------------------------- 
   Output File Mapping
 -----------------------------------------------------------------------------------------*/
	HANDLE outFile;
    HANDLE outFileMap;
    PVOID pvOutFile;
    DWORD dwOutFileSize;
	// Output Testfiles --------Note: Windows 7 won't allow writing to C:\ without admin rights -------------------------------------------------------------
   char outFilename[] = "c:\\temp\\outfile.txt";  
    argv[2] = "c:\\temp\\outfile.txt";

   //-----------------------------------------------------------------------------------------

   cout<<"enter Output path and Filename: ";
	//cin>>outFilename;
	cout<<endl;

    outFile = CreateFile(outFilename,
                         GENERIC_READ | GENERIC_WRITE,
                         0,
                         NULL,
                         CREATE_ALWAYS,
                         FILE_ATTRIBUTE_NORMAL,
                         NULL);

	if (outFile == INVALID_HANDLE_VALUE)
	{  
		cout << "outFile could not be opened.";
        return(FALSE);
     }

	outFileMap = CreateFileMapping(outFile,
                                     NULL,
                                     PAGE_READWRITE,
                                     0,
                                     dwInFileSize,
                                     NULL);
      if (outFileMap == NULL)
      {
            cout << "outFile map could not be opened";
            CloseHandle(outFile);
            return(FALSE);
      }

      pvOutFile = MapViewOfFile(outFileMap,
                               FILE_MAP_READ | FILE_MAP_WRITE,
                               0,
                               0,
                               0);
 
      if (pvOutFile == NULL)
      {
            cout << "Could not map view of pvOutFile";
            CloseHandle(inFileMap);
            CloseHandle(inFile);
            return(FALSE);
      }

      //Input should equal output filesize
	  dwOutFileSize = dwInFileSize;
	  

	  //Test for Filesize
      cout << "This states the size of the input and output file. dwOutFileSize is: " << dwOutFileSize << endl;


      // pointer to the beginning address
      PSTR InFileptr = (PSTR) pvInFile;
      PSTR outFileptr = (PSTR) pvOutFile;


	  // Strart of Palindrome Catcher.
	  char compare;
	  bool drome = false;
	  char tmpArray [600];
	  char tmpOutArray [600];
	  int incount = 0;
	  int outcount = dwOutFileSize;
	  int t = 0;
	  stack<char> stack;  //Useing #stack
	  queue<char> queue;

	  // read the values of the memory-mapped file
      // from start to finish placing values in
      //array
//-----------------------------------------	  
	 for (i = 0; i < dwInFileSize; i++) 
    {
		InFileptr[i];
	 }
	  for (i = 0; i < dwInFileSize; i++) 
    {
		stack.push(tmpArray[i]);
	 }


			//If a space is not found push all chars onto array & increase the input count	
		if (InFileptr[i] != ' ') 
		{
			stack.push(tmpArray[i]);
			queue.push(tmpOutArray[i]);
		//test for output ----------------
		//	stack.push(outFileMap[i]);
		//----------------------------------
			incount++;	
		}
   //If a space is found compare chars one by one with the same word in reverse 
		if (InFileptr[i] == ' ')
		{
			while (!stack.empty())
			{
					stack.top() = tmpArray[t];
					t ++;
					stack.pop() ;
					cout << "top:" << stack.top()<< endl;

					outcount --;
					int i = 0;


		//while (!stack.empty())
		//	if (tmpArray[t] != stack.pop()) return false;
		//	return true;
					//if (compare == InFileptr[i]);
					//{

					//}

				}
				
		}
	  }

	  //Cleanup
	  UnmapViewOfFile(pvInFile);
      CloseHandle(inFileMap);
      CloseHandle(inFile);
      UnmapViewOfFile(outFile);
      CloseHandle(outFileMap);
      CloseHandle(outFile);
}