HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppModerate

Class for holding person's information

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
personforclassholdinginformation

Problem

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 
#include 
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 > 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>id_number_;

}
void Persons::displayData(int n)
{

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

}

Solution

I avoid using namespace std;.

cout > 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 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>id_number_;

}


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

Code Snippets

cout << "Enter Number Of Persons:";
int n;     //This is the number of objects that the user wants to make.
cin >> n;
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);
}
std::vector<Person> persons(n);
int Persons count;//initializing the static member

Context

StackExchange Code Review Q#621, answer score: 16

Revisions (0)

No revisions yet.