Vector of pointers to class objects dynamic allocation and dellocation

[euklid]

SeppJ schrieb:

• What's that constructor of CSNP supposed to do? As of yet it just sets nchr to 0.

• Why is everything public in CSNP?

• What do all these names mean? CSNP, nchr, snpName, allele1? Do you think anybody can understand this? Do you think, you can still understand this in two weeks?

• Why is genInfo global? Global variables are rarely needed and introduce a huge potential for hard to find errors. I don't see any reason why it should be global in your program.

• A forward declaration for a function is rather unusual style. That's not an error it's just style. But it shows that you did not read the good literature on C++.

• I don't see any reason why genInfo stores pointers. You don't need a pointer container. Why not store CSNP instances in genInfo? You would get rid of all the problems you had in the first place and even gain performance and get code that is much better maintainable. That's three huge advantages for no costs at all!

• return 0; in main also shows that you don't know the usual style and the finer points of C++.

• A function called "display" usually wants to be an overloaded operator << instead.

• Your filehandling is just a mess:

• Why all the clears? If something bad happend just ignore it?

• Why the close? See my points about syle above.

• Why check for errors before doing anything that can produce an error and then go on as if no error could have happend? And why only check eof? Do the reading from the file the same way as you read from the stringstream in lines 49-50. This inconsitency in style let's me suspect that you just Copy&Pasted code from different sources without understanding it.

• First close then clear? Do you even understand what you are doing there?

• Speaking of different styles: Why do you start to use char-Arrays instead of strings?

• pCSNP->nchr=(int) atoi(tokens[0].c_str()); Everything you do in this line screams bad style! Why the cast? Why a C-style cast? Why atoi instead of reading a number from a stream?

• I have the feeling that you could parse the file with much less code and in a much more robust way but I'm not going to decipher what your code is actually doing. If you describe the file format maybe someone will post a better parser.

• Thank you so much SeppJ for your valuable critical points. Yes I am a beginner and I do not have enough experience in c++ programming. That is I am learning by doing. I looked for another's code as well.Your comments will be guiding me to learn stuffs more deeply and to improve these stuffs
• The variable names I have given do have their meaning in the area probably you are not related with. Its all about genetics.
• At the moment, my interest was to know if the deallocation was correct. If not what should I do? Any suggestions on reading the file and saving each row as class object and all rows as a vector for further analysis later, would be very appreciable.

ifstream FILE;
			FILE.open(fileName.c_str());
			while(!FILE.eof()){
			char cLine[256]; // current line
			FILE.getline(cLine, 256, '\n');
			string sLine=cLine;
			string buf;
			stringstream ssLine(sLine);
			vector<string> tokens;   
			while(ssLine>>buf)   
				tokens.push_back(buf);
			vector<CSNP*> genInfo;
           CSNP* pCSNP = new CSNP;    /*There is no delete pCSNP later*/ 
		   pCSNP->nchr=(int) atoi(tokens[0].c_str());
           pCSNP->snpName=tokens[1];
           pCSNP->allele1=tokens[2];
           pCSNP->allele2=tokens[3];
           // entering the element of genInfo
		   genInfo.push_back(pCSNP);  
		for (int i=0; i<genInfo.size(); ++i){
				delete genInfo[i];

• my fileName looks like:
  1 snp1 0 1
  3 snp2 0 2
  2 snp3 1 1
  2 snp4 2 2
  3 snp5 1 2
• Thank you so much in advance!

[SeppJ]

1. Your allocation and deallocation were correct but very susceptible to errors. In case of an exception your deallocation will be ignored and you have a memory leak. If you change the program flow you could accidentaly create paths that don't deallocate your objects.
If a container has ownership of an object use a container that deallocates the objects in its destructor. Thus, none of these problems will arise.
2. As I said: There is no point in using new or delete in your program. Just use a vector<CSNP> and you are fine.

If I have time I will look into this in more detail and post an alternative solution to your problem.

[euklid]

SeppJ schrieb:

1. Your allocation and deallocation were correct but very susceptible to errors. In case of an exception your deallocation will be ignored and you have a memory leak. If you change the program flow you could accidentally create paths that don't deallocate your objects.
If a container has ownership of an object use a container that deallocates the objects in its destructor. Thus, none of these problems will arise.
2. As I said: There is no point in using new or delete in your program. Just use a vector<CSNP> and you are fine.
If I have time I will look into this in more detail and post an alternative solution to your problem.

Thank you "SeppJ" for your answer! I would be thankful to you if you could write me the solution or give me some hints .

• Actually, I want to read files with at least one million SNP( rows) with at least 4 columns. That is why, as far as I learnt, allocation and deallocation is assumed to be very good for fast computing.
  The data contained in my file looks like :
  #nchr #snpName #allele1 # allele2
  1 snp1 0 1
  3 snp2 0 2
  2 snp3 1 1
  2 snp4 2 2
  3 snp5 1 2
• I have put all the variables in public because I have to use them all over the program and I don't know yet if it will be okay to put them in private. I would try to use them as private and then a friend function or something like that.

Thank you for your time and for your help.
best wishes
Euklid

[SeppJ]

euklid schrieb:

[*] Actually, I want to read files with at least one million SNP( rows) with at least 4 columns. That is why, as far as I learnt, allocation and deallocation is assumed to be very good for fast computing.

You are mistaken. Twice:
1. Dynamic memory allocation is waaaaay slower than static variables. But you need it here, because many operating system enforce a very small stack size (Windwos for example ~ 1 MB), there is no way to have a static array this size. And you don't know how many values will be in the file, thus you need dynamic allocation anyway.
2. Dynamic memory management in C++ does not mean using new and delete. You very very rarely need new and delete and never for the purpose of dynamic memory management. The vector does the dynamic stuff for you. And it is good at what it does. If it helps you can imagine that the vector uses new and delete internally (it does not, but that is another story).

[*] I have put all the variables in public because I have to use them all over the program and I don't know yet if it will be okay to put them in private. I would try to use them as private and then a friend function or something like that.

That is not an argument. That's either lazyness or bad design. If you need friend a lot, your classes are not well designed. If your functions need a lot of private data, maybe they should be methods of your class? Or maybe your class has data-members that don't belong in this class.

I would be thankful to you if you could write me the solution or give me some hints .

Here you are:

#include <iostream>
#include <fstream>
#include <vector>
#include <string>
#include <sstream>

using namespace std;

class CSNP
{
private:
  int nchr;
  string snpName;
  string allele1;
  string allele2;
  friend ostream& operator<<(ostream &out, const CSNP &csnp)
  {
    // This was your display-function. Look up "operator overloading" if you
    // don't understand what is going on here.
    return out << "chr No:"<< csnp.nchr<< ", SNP ID: "<< csnp.snpName 
               << ", allele1:" << csnp.allele1<< " and alelle2: "<< csnp.allele2;
  }
  friend istream& operator>>(istream& in, CSNP &csnp)
  {
    // This function was your lines 43-65.

    // Ignore the empty lines and lines starting with '#'
    string line;
    while (getline(in, line) and (!line.size() or line[0] == '#'));

    // Parse the line. Yes, it's this easy.
    stringstream line_parser(line);
    line_parser >> csnp.nchr >> csnp.snpName >> csnp.allele1 >> csnp.allele2;

    // If parsing failed set the error flags
    if (!line_parser) in.setstate(ios::failbit);

    return in;
  }
}; 

int main()
{
  // No need for any pointers or globals
  vector<CSNP> genInfo;
  {
    // This was the rest of your readFile function that was not covered above.
    // I won't even make a separate function for these four lines.
    CSNP value;
    ifstream file("test.map");
    while (file >> value)
      genInfo.push_back(value);
  }
  // Note the use of scoping. value and file do no longer exist at this point.
  // The ifstream was closed automatically upon its destruction

  // This has not really changed. Using unsigned to get rid of the compiler
  // warning, use the overloaded operator instead of the display function
  // and use '\n' instead of endl as endl would be '\n' << flush and we don't
  // need the flush.
  for (unsigned int i=0; i<genInfo.size(); ++i)
    cout << genInfo[i] << '\n';
}

This program does about the same things your old program did. See how it's only about 40 lines of code? And it is much more robust at the same time and should even be faster. Try it: Your program will often Segfault when there are illegal lines in the input file. Or sometimes even on comment lines.

Use the power of the standard library! And maybe you should get a good book. Germans can always say that there is no good German literature on C++ but there are so many good English titles: The C++ Primer, Thinking in C++ (free ebook), the books by Stroustrup (the inventor of C++) himself, the (a tad more advanced) books by Myers.

[euklid]

Thank you very much ,,SeppJ" for your kind efforts. One more question which I wanted to ask you is : Will it be a problem if I read a large file with at least (1000000) rows?
After your comments, I searched articles in Dynamic memory and found the following text.

Frequent allocation and deallocation of dynamic memory leads to heap fragmentation, especially if the application is running for long periods and allocates small memory chunks. Learn what the dangers of heap fragmentation are and how you can avoid this problem.A highly fragmented heap may have a large number of free memory blocks but they are small and non-contiguous.

Here was something more:

To avoid heap fragmentation, first of all, use dynamic memory as little as possible. In most cases, you can use static or automatic storage instead of allocating objects dynamically. Secondly, try to allocate large chunks rather than small ones. For example, instead of allocating a single object, allocate an array of objects at once, and use these objects when they are needed. If all these tips don't solve the fragmentation problem, you should consider building a custom memory pool.

I am also looking on the litrature you mentioned. Hopefully, it will help me a a lot for further progress.
Thank you once again and have a nice evening.
best wishes
Euklid

[SeppJ]

1000000 is a rather small number nowadays. If it were 1000000000 rows (or more generally: several Gigabytes of data) I would start thinking about something else. vector should not fragment the heap very much as it just allocates one large chunk of memory. It's your old scheme of allocating single objects and saving pointers to them that may cause heap fragmentation.

edit: You might also want to think about your CSNP-data-members: Is string the correct model for the kind of data you need for snpName and allele1? From your example file it looks more like an enum, char[4] or int. You should consider using a custom datatype for these.

[Skym0sh0]

i havent followed the thread yet, but about your question to read in 10...00 rows of data. consider the possibility to read only some less rows you really need isntead of the whole file...

[euklid]

Skym0sh0 schrieb:

i havent followed the thread yet, but about your question to read in 10...00 rows of data. consider the possibility to read only some less rows you really need isntead of the whole file...

What you mean by ,, consider the possibility to read only less rows?" I have to write a program to read nearly 1000,000 rows. I have read a file with nearly 960,000 using the code corrected by SeppJ.It took 14 seconds in my computer windows vista.
By the way, I have a question to SeppJ. You never closed the file ,,test.map" Isn't it necessary to close the file after reading? If not , how it works? Would you please mind explaining about it?
thank you and have a nice weekend.
Euklid

[cooky451]

euklid schrieb:

By the way, I have a question to SeppJ. You never closed the file ,,test.map" Isn't it necessary to close the file after reading? If not , how it works? Would you please mind explaining about it?

He mentioned that in line 55, the file is closed by the destructor.

[euklid]

Sorry "SeppJ", but I have to bother you once again.
Actually, my ,,test.map" file has two options. In the first case, test.map file looks like
**
#nchr #snpName #allele1 # allele2
1 snp1 0 1
3 snp2 0 2
2 snp3 1 1
2 snp4 2 2
3 snp5 1 2
**

In the second case: it looks like
**

#snpName #allele1 # allele2
snp1 0 1
snp2 0 2
snp3 1 1
snp4 2 2
snp5 1 2
**
Then I was trying to check this case on your code like following but it didn't work. Would you please mind looking at this code. Line 41 is not working. it gives an error message
**
Error: can't call member function "bool CSNP::checkCorrectMapfile(std::string )" without object.
**

#include <iostream>
#include <fstream>
#include <vector>
#include <string>
#include <sstream>
#include<cstdlib>

using namespace std;

class CSNP{
public:
	bool checkCorrectMapfile(string line);
private:
	static bool threeColumns;
	int nchr;
	string snpName;
	string allele1;
	string allele2;
	friend ostream& operator<<(ostream &out, const CSNP &csnp);
	friend istream& operator>>(istream& in, CSNP &csnp);
};

int main(){
  vector<CSNP> genInfo;{
    CSNP value;
    ifstream file("test.map");
    while (file >> value)
      genInfo.push_back(value);
  }
  for (unsigned int i=0; i<genInfo.size(); ++i)
    cout << genInfo[i] << '\n';
return 0;} 

/Defining class objects:
bool CSNP::threeColumns=false;

istream& operator>>(istream& in, CSNP &csnp){
    string line;
    while (getline(in, line) and (!line.size() or line[0] == '#'));
    stringstream line_parser(line);
    if(CSNP::checkCorrectMapfile(line)) // this line is not working 
	line_parser >> csnp.nchr >> csnp.snpName >> csnp.allele1 >> csnp.allele2;
	else 
		line_parser >> csnp.snpName >> csnp.allele1 >> csnp.allele2;
    if (!line_parser) in.setstate(ios::failbit);
    return in;}

bool CSNP::checkCorrectMapfile(string line){
	bool tfValue=false;
		stringstream ss(line);	  
	 string buff;
	  vector<string> tokens;
	 	while(ss>>buff)
     tokens.push_back(buff);
	if(threeColumns && tokens.size()!=3){
    	 cerr<<"your map file was expected to have 3 columns but this is not the case.";
		 exit(1);
	}
	if(!threeColumns &&tokens.size()!=4){
    		cerr<<"your map file was expected to have 4 columns but this is not the case";
			exit(1);
	}		
	if(threeColumns)tfValue=true;
	 return tfValue;}

 ostream& operator<<(ostream &out, const CSNP &csnp){
    return out << "chr No:"<< csnp.nchr<< ", SNP ID: "<< csnp.snpName
               << ", allele1:" << csnp.allele1<< " and alelle2: "<< csnp.allele2;
  }

[Gast]

class CSNP{
public:
    static bool checkCorrectMapfile(string line);
// ...
};

[euklid]

• Gugelmoser schrieb:

class CSNP{
public:
    static bool checkCorrectMapfile(string line);
// ...
};

Hi Gugelmoser, thank you so much! would you please explain what kind of miracle this static in the code

static bool checkCorrectMapfile(string line);

did and it worked.why it didn't work without static?

best wishes
euklid

[Gast]

euklid schrieb:

• Gugelmoser schrieb:

class CSNP{
public:
    static bool checkCorrectMapfile(string line);
// ...
};

Hi Gugelmoser, thank you so much! would you please explain what kind of miracle this static in the code

static bool checkCorrectMapfile(string line);

did and it worked.why it didn't work without static?

• I tried with static but checkCorrectMapfile is still not doing what I want this function to do. Even though I have four columns in the map file, it is still saying that:
  "your map file was expected to have 4 columns but this is not the case"
  I do not know why.

best wishes
euklid

class Person
{
    private:
        double weight;

    public:
        Person(double w) : weight(w) {}
        double whats_my_weight() { return weight; };    
};

int main()
{
    /*
        In general, you'll always have to greate an object.
        Look, what sense would it make if you do Person::whats_me_weight()... if you have no person, you can't have any weight.
    */
    Person p(88.5);
    cout << "I weight " << p.whats_my_weight();

    /*
        But sometimes you want a function to be independent from a object... then static comes into play, like in your case.
        You want to call a function with its class name using the scope operator: you want to do CSNP::checkCorrectMapfile(line)
        --> thus you want a function being independent from an object.
    */
}

I hope that helps a bit.

[euklid]

class Person
{
    private:
        double weight;
    
    public:
        Person(double w) : weight(w) {}
        double whats_my_weight() { return weight; };    
};

int main()
{
    /*
        In general, you'll always have to greate an object.
        Look, what sense would it make if you do Person::whats_me_weight()... if you have no person, you can't have any weight.
    */
    Person p(88.5);
    cout << "I weight " << p.whats_my_weight();
    
    
    /*
        But sometimes you want a function to be independent from a object... then static comes into play, like in your case.
        You want to call a function with its class name using the scope operator: you want to do CSNP::checkCorrectMapfile(line)
        --> thus you want a function being independent from an object.
    */
}

I hope that helps a bit.

Thank you so much! I appreciate your explanation!

• I tried with static but checkCorrectMapfile is still not doing what I want this function to do. Even though I have four columns in the map file, it is still saying that:
  "your map file was expected to have 4 columns but this is not the case".
• I found that vector<string> tokens has 0 size. This means there is no stream any more in when I call this function. But I still do not know what to do. I think after line 40 of the code

stringstream line_parser(line);

I should not write

if(CSNP::checkCorrectMapfile(line))

Probably the line has no value any more. However, I still do not know how to solve this problem.

[Gast]

edit: Murks

[euklid]

thank you so much for kind response. it works fine but in this way, I have to open and read the file two times: once when I want for checkCorrectMapfile and once for the function

ostream& operator<<(ostream &out, const CSNP &csnp)

since my file is very large, I am interested to read the file only once to check if the correctness of test.map and then save it as an object of class type.
using the following function.

stream& operator>>(istream& in, CSNP &csnp){
    string line;
    while (getline(in, line) and (!line.size() or line[0] == '#'));
    stringstream line_parser(line);
    if(CSNP::checkCorrectMapfile(line)) // this line is not working
    line_parser >> csnp.nchr >> csnp.snpName >> csnp.allele1 >> csnp.allele2;
    else
        line_parser >> csnp.snpName >> csnp.allele1 >> csnp.allele2;
    if (!line_parser) in.setstate(ios::failbit);
    return in;}

However, it is not working. I have written this problem as a simple example. If anyone could find the solution of the following problem, I would be thankful.
The problem is is written as comments in lines 22, 23 and 27 of the following program

#include<iostream>
#include<sstream>
#include<string>
#include<vector>
#include<cstdlib>
using namespace std;
int main(){
	string line;
	string name, famName, buff;
	vector<string> tokens;
	cout << " enter your name: ";
	while(cin){
		getline(cin, line);
		if(line=="0")exit(1);
	cout << "your name is: "<< line <<".\n"; 
	stringstream ss(line);
		while (ss>> buff)
		tokens.push_back(buff);
		for(int i=0; i<tokens.size();i++)
			cout <<" "<<tokens[i];
			cout <<endl;
		/* I want to use ss again but it is not working 
		 Is the life time of ss expired now?
		 */
		ss>>name >> famName;
	cout <<"name: " <<name << " and "<<"famName: "<<famName <<".\n";
        // name and famName have  no values assigned. why?       

	}	
return 0;
}

ps: There is no compiling error. Only that name and famName have no values assigned at the end.

[Gast]

Just to clarify:

1. There is a mapfile given just like that:

1 snp1 0 1
3 snp2 0 2
2 snp3 1 1
2 snp4 2 2
3 snp5 1 2

2. You want to check wheather the mapfile is built correctly.

3. You want to read the mapfile and store its content into CSNP-Objects.

4. You want to output all the CSNP-Objects.

Is this what you're trying to do or are we tearing past each other?

[euklid]

Gugelmoser schrieb:

Just to clarify:
1. There is a mapfile given just like that:

1 snp1 0 1
3 snp2 0 2
2 snp3 1 1
2 snp4 2 2
3 snp5 1 2

2. You want to check wheather the mapfile is built correctly.

3. You want to read the mapfile and store its content into CSNP-Objects.

4. You want to output all the CSNP-Objects.

Is this what you're trying to do or are we tearing past each other?

Yes Gugelmoser, you are absolutely right.
[list]
[1] I have a bool parameter with default value false say

bool threeColumns =false

if threeColumns=true, the map file looks like

snp1 0 1
 snp2 0 2
 snp3 1 1
 snp4 2 2
 snp5 1 2

In this case I want that the default value of #nchr is for example 0.

[2]If threeColumns=false, then

1 snp1 0 1
3 snp2 0 2
2 snp3 1 1
2 snp4 2 2
3 snp5 1 2

[3] I want first check for the correct mapfile like I did before and then I want to save them as class a vector of class objects.
[4] The map file is very large at least 10000,000 rows.

int main()
{
  vector<CSNP> genInfo;
  {
    CSNP value;
    ifstream file("test.map");
    while (file >> value)
      genInfo.push_back(value);
  }

where each object CSPP looks like

class CSNP
{
private:
  int nchr;
  string snpName;
  string allele1;
  string allele2;
  friend istream& operator>>(istream& in, CSNP &csnp);
// this I was trying to define like before. 

 };

[Gast]

Well i was bored and that's the result (you're looking for)

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

/*
    mapfile:
        1 snp1 0 1
        3 snp2 0 2
        2 snp3 1 1
        2 snp4 2 2
        3 snp5 1 2
*/

class CSNP
{
    private:
        static bool three_columns; 
        int nchr;
        string snpName;
        string allele1;
        string allele2;

    public:
        static bool checkCorrectMapfile(const string&);
        CSNP() : nchr(0) {}
        friend istream& operator>>(istream& in, CSNP& csnp) 
        {
            if(!three_columns)
            {
                in>>csnp.nchr;
            }
            getline(in,csnp.snpName,' ');
            getline(in,csnp.allele1,' ');
            return getline(in,csnp.allele2);
        }
        friend ostream& operator<<(ostream &out, const CSNP &csnp)
        {
            return out << csnp.nchr << ' ' << csnp.snpName << ' ' << csnp.allele1 << ' ' << csnp.allele2;
        }      
};

bool CSNP::three_columns = false;
bool CSNP::checkCorrectMapfile(const string& mapfile)
{

    ifstream in( mapfile.c_str() );

    if(!in)
    {
        cerr << "could not open mapfile";
        return false;
    }
    else
    {
        vector<string> lines;
        string buffer;

        while( getline(in,buffer) )
        {
            lines.push_back(buffer);
        }

        for(vector<string>::iterator it=lines.begin(); it!=lines.end(); ++it)
        {
            istringstream iss(*it);
            vector<string> tokens;

            while( getline(iss,buffer,' ') )
            {
                tokens.push_back(buffer);
            }

            if(tokens.size() != 3 && tokens.size() != 4)
            {
                cerr << "your mapfile was expected to have either 3 or 4 columns!";
                return false;
            }

            if( !three_columns && tokens.size() == 3)
            {
                three_columns = true;
            }
        }
    }
    cout << "your mapfile is correct!";
    return true;
}

int main()
{
    string mapfile = "...";

    if( CSNP::checkCorrectMapfile(mapfile) )
    {
        ifstream in( mapfile.c_str() );
        vector<CSNP> objects;

        for(CSNP csnp; in >> csnp;)
        {
            objects.push_back(csnp);
            cout << '\n' << csnp;
        }

        // now, every line is stored into a CSNP-Object.
    }

    return 0;
}

To be honest, i don't know wheater the design is or is not good.
Anyhow, reading the file only once... i didn't manage to get any good design for that :(.

[euklid]

thank you so much for your help. I have posted new code which is error free but not efficient.