# Class for holding person's information

I have written another program in C++. Excluding the point that the class definition should be in a separate header file, is there anything that needs to be improved?

/*
Date:5th January 2011
*/
#include <iostream>
#include <string>
using namespace std;
class Persons     //A class that will store the name,addresses and id numbers of the users
{
private:
string name_;
int id_number_;

public:
static int count;    //This is the count of the objects created
Persons();
void getData(int n);
void displayData(int n);
~Persons();
};

int Persons count;//initializing the static member

int main()
{
cout << "Enter Number Of Persons:";
int n;     //This is the number of objects that the user wants to make.
cin >> n;
Persons *ptr;    //A pointer that will be used for the dynamic memory allocation.

/*Exception Handling*/
////////////////////////////////////////////////////////////////////
try
{
//ptr=new [sizeof(Persons) * n];
ptr=new Persons[n];
}
{
cout<<"Sorry,Program Can Not Continue";
cin.get();
exit(1);
}
/////////////////////////////////////////////////////////////////////
for(int i = 0; i< n; i++)
{
ptr[i].getData(n);
}
for(int j = 0; j< n; j++)
{
ptr[j].displayData( n );
}
cin.get();
delete[] ptr;
return 0;
}

/*Function Definitions*/

Persons::Persons()
{
name_="";
id_number_=0;
count++;
}
void Persons::getData(int n)
{

cout<<"Enter Name (Press '$' To Exit):"; getline(cin,name_,'$');
cout<<endl<<"Enter Address (Press '$' To Exit):"; getline(cin,address_,'$');
cout<<endl<<"Enter Identitiy Card Number:";
cin>>id_number_;

}
void Persons::displayData(int n)
{

cout<<"Name:"<<name_;
cout<<endl<<"Identitiy Card Number:"<<id_number_;

}


I avoid using namespace std;.

cout << "Enter Number Of Persons:";
int n;     //This is the number of objects that the user wants to make.
cin >> n;


Personally, I always use getline to capture a line at a time when using std::cin for interactive input. The fact that >> stops at whitespace means that it is very easy for the user to enter two fields in a single line and then the second field gets used at a subsequent point in the program without the user being able to respond to a later prompt.

Also you should always check whether the input operation succeeded.

std::string inputline;
if( std::getline( cin, inputline ) )
{
std::istringstream iss( inputline );
if( iss >> n )
{
// success
}
else
{
}   // fail
}
else
{
// fail
}


catch(bad_alloc xa)
{
cout<<"Sorry,Program Can Not Continue";
cin.get();
exit(1);
}


For all that you actually do in the exception handler, you may as well let the expection propogate. The runtime might actually give a more meaningful error message for bad_alloc. Also, unless you have a good reason, always catch exception by const reference. e.g. catch( const std::bad_alloc& ba ).

Looking at your definition of Persons, I think that Person is a better name for the class. I see one name and one address member, if the class represented multiple people (e.g. a household) you would need at least multiple names, I would have thought.

Always try and avoid needing to use delete[] explicitly. Here a vector would be much simpler and safer.

std::vector<Person> persons(n);


int Persons count;//initializing the static member


Class static variables are normally not advisable. This isn't valid, it should be:

int Persons::count;


In this program it isn't giving you anything that n isn't so I would just remove it.

Persons::Persons()
{
name_="";
id_number_=0;
count++;
}


Always prefer to initialize member variables. Assigning "" to a newly constructed std::string is redundant so you could do either.

Persons::Persons()
: name_()
, id_number(0)
{
count++;
}


or even this (although some people prefer an explicit initializer for all members and bases).

Persons::Persons()
: id_number(0)
{
count++;
}


for(int i = 0; i< n; i++)
{
ptr[i].getData(n);
}
for(int j = 0; j< n; j++)
{
ptr[j].displayData( n );
}


Why do getData and displayData take an int? They don't use the parameter, and I don't understand why you are passing n.

cin.get();


This seems redundant.

void Persons::getData(int n)
{

cout<<"Enter Name (Press '$' To Exit):"; getline(cin,name_,'$');
cout<<endl<<"Enter Address (Press '$' To Exit):"; getline(cin,address_,'$');
cout<<endl<<"Enter Identitiy Card Number:";
cin>>id_number_;

}


Again, my comments about the unused parameter, always using getline and always checking the success of input operations all apply.

• The class does represent multiple people.If you have a closer look to the new function used.I take in the number of people the user wants to enter.Thanks a lot for the descriptive answer.
– user1211
Feb 5 '11 at 16:58
• @fahad: Does it? When you ask how many "Persons" you then create that number of Persons objects. Doesn't that mean you have one Persons per "Person" (if that makes sense) ? Feb 5 '11 at 17:01
• Oh sorry!I agree,even if the object is one or hundred,each and everyone of them will be modeling individual person.
– user1211
Feb 5 '11 at 20:06
• Can you please tell me why should I avoid using namespace std?
– user1211
Feb 5 '11 at 20:08
• @fahad: To some extent it's a matter of style, but the link I provided contains my justification. stackoverflow.com/questions/1265039/using-std-namespace/… Feb 5 '11 at 20:13

First the errors: the static member attribute has to be initialized as:

int Persons::count = 0;


Then in general: you should use std::vector to avoid having to manually manage resources. While they are correctly managed in this small code sample, if you start building bigger projects you will most probably make a mistake some day (all of us do it). Program defensively to avoid errors: use existing libraries.

The getData() and displayDAta() member functions do not need the argument, nor they use it. Remove it from the interface.

You are not handling errors in the getData() member function nor in main. You should check the state of the istream after each operation.

I tend to avoid using namespace directives, but this is a matter of taste.

• I guess the static member initializes itself to 0
– user1211
Feb 5 '11 at 16:51

if you need to comment about the use of a variable like:

static int count;    //This is the count of the objects created


you should use self descriptive names like object_count instead.

same goes for

int n;     //This is the number of objects that the user wants to make.


which could e amount_of_objects ore something like that

• Persons name is quite confusing to represent a single person

• It looks like the main idea is to serialize/deserialize instance of Person (correct me if I'm wrong here). The more natural is to override operator >> and operator << - it would allow you to specify every stream you want as data source/destination not only standart out/in.

istream& operator>> (istream& in, Person& p)
{
in >> p.Name
return in;
}

ostream& operator<< (ostream& out, const Person& p)
{
out << p.Name << 't';

This probably isn't going to be the case, but is this class ever going to be used in a multithreaded application? If so, you should be locking around the incrementing of count since multiple threads could be allocating an object of that type at the same time. In general, be careful when doing that sort of thing though and ask yourself if your class really cares how many times its been constructed, or if you should be keeping track outside of the class itself.