0

I have no idea about C++, but I've been assigned to edit this piece of code:

// Setup path information for output file from environmental variables
char * path = new char[100];
path = getenv("MODEL_MERGE");
char * templatePath = new char[100];
char * outputPath = new char[100];

strcpy(templatePath, path);
strcat(templatePath, "infile location");
strcpy(outputPath, path);
strcat(outputPath,"outfile location");
cout << "temp: " << templatePath << endl;
cout << "out:  " << outputPath << endl;

//input output file streams for reading/writing to files
ifstream readFile(templatePath);
ofstream outFile(outputPath); 

My goal is to replace the "infile location" and "outfile location", which currently point to specific files. I want the user to be able to enter the file names when running from command prompt. Sorry if this is something as simple as <<cin, but I couldn't get that to work, and I have zero experience with this language.

Got it! Everything above was replaced by:

//User inputs paths
    string input;
    string output;
    cout<<"Input path?"<<endl;
    cin>> input;
    cout<<"output path?"<<endl;
    cin>> output;   


//input output file streams for reading/writing to files
ifstream readFile(input.c_str());
ofstream outFile(output.c_str());`

Thanks everyone for the help!

Errorum
  • 223
  • 4
  • 16
  • 1
    If you aren't familiar with c++ then you should start by porting everything over to using `std::string` – shuttle87 Aug 17 '15 at 22:15
  • Can you show us exactly what you tried, and explain what happened (compile error? segmentation fault? empty output?) – Jim Lewis Aug 17 '15 at 22:29
  • [cin is a simple way to do it.](http://en.cppreference.com/w/cpp/io/cin) It would be `cin >> ;` at a very minimum. Should also be a bunch of tests for validity to make sure the person at the command line isn't about to do something silly or malicious. Definitely replace the char arrays with strings. Why are they being `new`ed anyway? – user4581301 Aug 17 '15 at 22:50

1 Answers1

2

There is enough wrong with the code supplied to OP to be worth a quick going over in addition to pointing the OP in a useful direction.

First, no test for NULL on the call to getenv. If MODEL_MERGE doesn't exist, NULL is returned and then used in string copies. BOOM!

Second, newing all those arrays. Dynamically allocate only as a last resort. new must be pared with at least one delete, depending on the code's flow, to return the allocated memory for reuse when no longer needed. Since there seems to no need to dynamically allocate and the sizes of the arrays are known, they should have been defined as char templatePath[100];. Less memory management to be dealt with and effectively no possibility of leakage.

Third renders point two obsolete. Rather than using char arrays, use strings where possible. Not only do they handle all of the memory management, including resizing as needed rather than trampling out of bounds, for you, they also perform routine tasks like copying and appending with much less fuss. This bit I'll demonstrate below.

Proper use of cin and cout is well detailed on a number of sites so I won't go over it here.

Also note I've removed the need for using namespace std; by explicitly stating the namespace at use. Read why using namespace std; is often a bad idea.

#include <fstream>
#include <iostream>

int main()
{
    char * Model_MergePath = getenv("MODEL_MERGE");
    if (Model_MergePath != NULL)
    { //MODEL_MERGE is defined
        std::string path(Model_MergePath); //replace icky and fault-prone char array
        std::string templatePath = path; // copy strings with =
        std::string outputPath; // not assigning path here so I can demonstrate 
                                //something else later

        std::string inFileLoc; // new throw away variables for user input.
        std::string outFileLoc; // could use the same var for both. I didn't for clarity

        std::cin >> inFileLoc; // get input
        templatePath += inFileLoc; // append to strings with += 
        std::cin >> outFileLoc;
        outputPath = path + outFileLoc; // concatenate strings with +

        // validate paths for correctness and possible intrusion attempts here
        // this, I'm afraid, is up to the OP as the security requirements are unknown

        std::cout << "temp: " << templatePath << std::endl;
        std::cout << "out:  " << outputPath << std::endl;

        //input output file streams for reading/writing to files
        std::ifstream readFile(templatePath); 
        // older C++ compilers may require a c-style string as the file path
        std::ofstream outFile(outputPath.c_str());

        // do stuff with readFile and outFile

        // remove the deletes that should have corresponded to the replaced `new`s
        return 0;
    }
    else
    { //MODEL_MERGE is NOT defined
        std::cerr << "Cannot find environment variable MODEL_MERGE. Exiting." << std::endl;
        return -1;
    }
}
Community
  • 1
  • 1
user4581301
  • 33,082
  • 7
  • 33
  • 54
  • Wow, thanks for the in-depth info. I should have mentioned that the piece I posted is just the start of a massive script; I just care about the IO portion. Not sure if that is why you saw so many issues with it. Is the `do stuff with readfile and outfile` line where I'm supposed to put the rest of the code? When I try that, then compile from cmd, I get a C2664 error for converting strings to const char *. Also, I get a bunch of C2065 errors (undefined identifiers) on all the IO path variables. – Errorum Aug 18 '15 at 18:06
  • @Errorum You should care about the rest of the script. The first two lines you posted hints that it was written by someone also didn't understand the basics of C or C++ programming. You can fix up your corner, but you will be unable to test your corner properly because you won't know which are your errors and which are your predecessors. Calling the `c_str()` method on a string returns the contents of the string as a `const char *` and should solve the C2664 errors. The C2065 errors require context, but you should be able to find what identifier has not been defined and investigate why. – user4581301 Aug 19 '15 at 14:30
  • Hey, thanks again for all your advice. I spent a day just learning the basics of C++, and you were completely right. I realized cin/cout were all I needed. Everything I posted was replaced by four lines of code – Errorum Aug 21 '15 at 19:56