Is this code efficient for file parsing?

Question

Is the below code efficient for parsing the file, or do I face performance issues? If the latter, please offer suggestions.

#include <iostream>
#include <fstream>
#include <map>

using namespace std;
ofstream outfile;
void writedata(string data, string file)
{
    outfile.open(file.c_str(), ios::app);
    outfile << data;
    outfile.close();
}

int main()
{
    ifstream input("data.txt", ios::in);

    ofstream out;
    string line;
    string tmp;
    string data;
    string filename;
    string name;
    size_t found;
    map<string, string> MyFileMap;
    map<string, string>::iterator it;

    while (input >> line)
    {
        found = line.find_last_of("pmf");
        if (found != string::npos)
        {
            name = line.substr(found + 1, line.length());
            filename = line.substr(found + 1, line.length());
            cout << "filename without txt " << filename << endl;
            for (it = MyFileMap.begin(); it != MyFileMap.end(); ++it)
            {
                if (name.compare(it->first) == 0)
                {
                    while (input >> tmp && tmp.compare("pme"))
                    {
                        data += tmp;
                    }
                    cout << "Data push " << data << " to " << it->second << endl;
                    writedata(data, it->second);
                    data.clear();
                    break;
                }
            }
            if (it == MyFileMap.end() || MyFileMap.empty())
            {
                cout << "No match in map" << endl;
                filename = filename + ".txt";
                cout << "Filename " << filename << endl;
                while (input >> tmp && tmp.compare("pme"))
                {
                    data += tmp;
                }
                cout << "Data push " << data << " to " << it->second << endl;
                out.open(filename.c_str());
                MyFileMap[name] = filename;
                for (it = MyFileMap.begin(); it != MyFileMap.end(); ++it)
                {
                    if (name.compare(it->first) == 0)
                    {
                        writedata(data, it->second);
                        data.clear();
                        break;
                    }
                }
            }
        }
    }
    system("pause");
}

sample file : data.txt

pmfwork1
data1
data1
pme
pmfwork2
data2
data2
pme
pmfwork3
data3
data3
pme
pmfwork4
data4
data4
pme

In the above code, I am trying to parse the sample file with pmf and pme as begin and end flags, work as the file name, and data as file contents. Below is the sample file.

I would like to reduce the use of open call for multiple times. It would be of great help if you provide any suggestions.

Above code is generating four different text files: work1, work2 and so on.

Answer 1

1. ofstream outfile;
   

   Why is this declared global? The variable is only used in writedata. Declare it in this scope.

2. ofstream out;
   

   What is the purpose of this stream? It is opened (multiple times!) in the code but never written to.

3. string tmp;
   string data;
   string filename;
   string name;
   size_t found;
   

   You should declare objects in the narrowest scope that they need to exist, instead of the function head.

4. while (input >> line)
   

   The operator >> will read one word at a time, not a whole line. If you want to read whole lines, you should use while(std::getline(input, line)).

5. found = line.find_last_of("pmf");
   

   I think this doesn't do what you expect it to do. It finds the position of the last character in line which is equal to either of the single characters in "pmf". You probably want to use line.rfind("pmf") instead, which returns the position of the last occurrence of the full string "pmf".

   Looking at your example file however I believe you actually want to check whether the beginning of the line is "pmf" (consider the work string might include "pmf"), so you would use line.find("pmf") and check it against 0 instead of string::npos.

6. name = line.substr(found + 1, line.length());
   filename = line.substr(found + 1, line.length());
   

   These two strings are the same? Why? In the following code they are used interchangeable, so stick to one and remove the other. Even if they were used differently you could still use filename = name.

   Also you are trying to get a substring of the same length as the whole line (the second argument to substr is the length of the substring to be extracted), which obviously doesn't work. However substr truncates in this case such that a substring ending at the end of the input string is returned.

   The supposed filename you are extracting should be the part after pmf in pmfwork1. However you start the substring one position after the position of the last occurrence of pmf, which would be result in "mf" being part of the string. However with the particular example file you provided and in combination with the semantic error I laid out in 5., you actually get the expected result. Here two errors cancel each other. For different input however you would have very unexpected results.

7. for (it = MyFileMap.begin(); it != MyFileMap.end(); ++it)
       if (name.compare(it->first) == 0)
   

   The whole purpose of a map is to be able to access the values via a key instead of iterating over them. You should use this functionality:

   auto it = MyFileMap.find(name);
   if(it != MyFileMap.end())
       // "name" found it is at "it"
   else
       // "name" is not in "MyFileMap"!
   

8. while (input >> tmp && tmp.compare("pme"))
   

   Again the input is one word each instead of lines. Data which includes the word "pme" would trigger the end.

9. if (it == MyFileMap.end() || MyFileMap.empty())
   

   The second condition is redundant. If MyFileMap.empty() is true, then also it == MyFileMap.begin() == MyFileMap.end() at this point.

10. cout << "Data push " << data << " to " << it->second << endl;
    

    Here you try to dereference it, which you previously checked to be MyFileMap.end(). Dereferencing the end() iterator invokes undefined behaviour. Since I guess these lines are debug output only anyway, just remove that part.

11. MyFileMap[name] = filename;
    for (it = MyFileMap.begin(); it != MyFileMap.end(); ++it)
    {
        if (name.compare(it->first) == 0)
        {
            writedata(data, it->second);
            data.clear();
            break;
        }
    }
    

    What is the purpose of this loop? You just inserted the key name into the map and now you search it again. Why not simply access it directly:

    MyFileMap[name] = filename;
    writedata(data, MyFileMap[name]);
    data.clear();
    

    Note however that this is the same as, but in general slower than:

    MyFileMap[name] = filename;
    writedata(data, filename);
    data.clear();
    

12. system("pause");
    

    In order to use std::system you need to #include<cstdlib>.

13. What is the purpose of the MyFileMap at all? It stores the names as keys and filenames as values. The only difference between the two is that filename has an ".txt" added. Why do you need to save this? You can append the ".txt" each time you have a new name. This would be much easier to read and much faster.

14. If you are trying to minimize the number of open calls you might want to store all the opened ofstreams in the map and access them as you need them, closing all at the end of the function. Notice however that ofstream is not copyable, so you need to store references to them in the map, instead of the objects themself.

    If the input files are not that big, you might also consider saving all the parsed data in memory first and then writing it to the output files in one go. While parsing you can use a string-string map to save the data for each file.

15. You are lacking any check of syntax errors in the input file. You probably should add code that reports back if the syntax is not as expected instead.

In summary:

Your code will likely not do what you expect it to do in many cases. You should generate more diverse test cases. (try pmfworkm instead of pmfwork1 in your example file and report what happens).

Several parts of the code are redundant (all the map-iterating loops) and at the same time they might be very time costly for large input files (since the map might grow rapidly).

Answer 2

Is the below code efficient for parsing the file, or do I face performance issues?

If you approach performance problems like this you will be unable to implement any effective optimizations.

There are two ways to tackle optimizing code:

• the first one is to make sure you avoid efficiency worst cases and pitfalls (prefer integers (or enums) as keys in maps instead of strings for example). This applies to all code you write.

• the second one is to do these steps, in this order:

  1. establish speed requirement for module.

  2. measure current performance (i.e. measure what your biggest processor hogs are in the affected code)

  3. optimize code focusing on the problem areas

  4. go to step 2 and repeat until speed requirement is met.

Regarding the first way (generic optimizations in written code), your code could use the following improvements:

• only declare variables right before you use them; this way, in the case of an early return from the function (or in the case the local scope is avoided because of an alternate code path) the constructor code will not be executed at all. The most optimal code possible is the one that isn't executed at all.

• do not declare global variables.

• remove unused and redundant variables

Regarding the second way of optimizing (what your question seems to be actually about) you have the following steps yet do to:

1. establish speed requirement for the code (i.e. "how optimized do you need it to be")

2. measure the current efficiency of the code.

Until you do this, it may be that optimization is unnecessary and a waste of time.

As a side note (and not related to your question - just a pet-peeve): please stop putting using namespace std in the global scope. At the very least (if you do use it) use it in function scope, not in file scope - it's bad practice).