Writing waypoints to a file

I have generated code which reads a file containing ways, take the individual waypoints and writes them into a new file, but only if the waypoint is not already present. At this moment my program stores the waypoint in a vector. Before the new found waypoint is written to the file the program checks if the waypoint is not already in the vector.

The program read and wrote the first 140,000 waypoint is 12 minutes, but as there are more then 13 million ways (expected 90 million waypoints) to read, examine and written this is going to take days.

I would much appreciate if anyone of you could give me a suggestion to improve the code.

```
struct NodeTemplate{
string ID;
string NodeID;
string latitude;
string longitude;
};
vector NodesOut;

void split(const string& s, char c,
vector& v) {
string::size_type i = 0;
string::size_type j = s.find(c);

while (j != string::npos) {
v.push_back(s.substr(i, j-i));
i = ++j;
j = s.find(c, j);

if (j == string::npos)
v.push_back(s.substr(i, s.length()));
}
}

int main(int argc, char *argv[])
{
string filenameIn="ways.txt";
ifstream myfile ((filenameIn).c_str( ));

string fileNameNodes2="nodes2.txt";
fstream myfile3;

myfile3.open(fileNameNodes2.c_str(),fstream::out | fstream::trunc);
myfile3.close();

myfile3.open(fileNameNodes2.c_str(),fstream::out | fstream::app);

if (myfile.is_open())
{while ( getline (myfile,line1) )
{

string s1=line1;
vector v1;
split(s1, ';', v1);

string s2=v1[4];
vector v2;
split(s2, ',', v2);
for (int t=0;t<v2.size();t++)
{
NodePresent=false;
for(int tt=0;tt<NodesOut.size();tt++)
{

Structure

Hmm.

OK. Property bag so its OK

struct NodeTemplate{
  string ID;
  string NodeID;     
  string latitude;
  string longitude;     
};

But are lat/long really string? Would they not be numbers? Also ID sounds like a number type thing.

Global Variables:

vector  NodesOut;

Global variables are bad. Don't do this.

They make the code hard to test. Side affects will cause problems.

Prefer return over out parameters.

void split(const string& s, char c,
           vector& v) {

Out parameters are hard to spot and maintian. Also its not very expensive to return a vector (there is no copy the compiler will elide it (assuming you enabled optimization)). Check out RVO and NRVO.

Also in C++11 and C++14 move semantics kick in and even if you can't optimize it with RVO or NRVO then it will be moved back to the calling function rather copied.

Standard functions

A lot of this work made trivial by the standard functions and some lambda's

while (j != string::npos) {
      v.push_back(s.substr(i, j-i));
      i = ++j;
      j = s.find(c, j);

      if (j == string::npos)
         v.push_back(s.substr(i, s.length()));
   }
}

How about:

// Use the string as a stream
std::stringstream stream(s);
// Copy "Item" objets from the stream into the vector.
std::copy(std::istream_iterator>(stream), std::istream_iterator>,
          std::back_inserter(v))

Not you can simply define an Item.

template
struct Item
{
    std::string   value;
    // Converts an Item into a string
    operator std::string const& {return value;}
    // Reads one item from the stream.
    friend std::istream& operator>>(std::istream& str, Item& data)
    {
        return std::genline(str, data.value, c);
    }
}

Use constructors.

string filenameIn="ways.txt";
      ifstream myfile ((filenameIn).c_str( ));

Much easier to write as:

ifstream myfile("ways.txt");

Also C++11 allows you to construct the stream object with a std::string so you don't need to call c_str() if you really want to use the variable name.

I don't see the point of opening and closing the file to truncate it.

myfile3.open(fileNameNodes2.c_str(),fstream::out | fstream::trunc);
      myfile3.close();

      myfile3.open(fileNameNodes2.c_str(),fstream::out | fstream::app);

By default a file will truncate an existing file. So all the above can be replaced by the single line:

std::ofstream  myfile3("nodes2.txt");

This is a useless test:

if (myfile.is_open())

I would be OK if you used it to generate an error message. But as it stands it is just protecting the while loop. You don't need to do that. The while loop will just not be entered because getline() will fail to read from an empty file.

As discussed above:

string s1=line1;   // usless copy.
                    vector v1;
                    split(s1, ';', v1);

Much easier to write as:

vector v1 = split(line1, ';');    
                    vector v2 = split(v1[4], ',');

Though personally I would probably have written an input operator that read everything directly from the stream. Rather than copy strings all over the place.

Conversion is hurting.

if(atoi(v2[t].c_str())==atoi((NodesOut[tt].ID).c_str()))

You are converting this every time!!!!!
You should convert it once when you read it in. Conversion is really expensive. (I mean the calls atoi())

Don't manually flush the stream

myfile3.flush();

This will cause very inefficient use of the underlying buffer. Let the stream flush itself. It is much better at the job then you are.

Better Soltion

This is how I would expect the code to be written.

class DataNode
{
     // Values 
     //   With the correct types NOT string
     friend std::istream& operator>>(std::istream& str, DataNode& data)
     {
          // read from the stream into a Data Node  "data".
          // Just one object.
          return str;
     }
     friend std::ostream& operator  dataNodes;

    while(input >> data) {
    {
        if (!dataNodes.find(data))
        {
            dataNodes.insert(std::move(data));
        }
    }

    std::ofstream    output("outputfile");
    std::copy(std::begin(dataNodes), std::end(dataNodes),
              std::ostream_iterator(output));
}

StackExchange Code Review Q#101725, answer score: 7