Class for holding person's information

Question

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
    Programmer:Fahad
*/
#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_;
        string address_;
        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];
    }
    catch(bad_alloc xa)
    {
        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_="";
    address_="";
    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<<"Address:"<<address_;
        cout<<endl<<"Identitiy Card Number:"<<id_number_;

}

Answer 1

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_="";
    address_="";
    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_()
    , address_()
    , 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.

Answer 2

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.

Answer 3

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

Answer 4

• 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    
    in >> p.Address;        
    return in;
  } 
  
  ostream& operator<< (ostream& out, const Person& p)    
  {    
       out << p.Name << 't';    
       out << p.Address << endl;    
       return out;    
  }
  

Answer 5

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.

Answer 6

When you are including a static count, include a copy constructor which also increments the static variable. If you had used vector instead of array, your count variable will not give the correct count. Also make sure you decrement in destructor.