patterncppModerate
Class for holding person's information
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
Personally, I always use
Also you should always check whether the input operation succeeded.
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
Looking at your definition of
Always try and avoid needing to use
Class
In this program it isn't giving you anything that
Always prefer to initialize member variables. Assigning
or even this (although some people prefer an explicit initializer for all members and bases).
Why do
This seems redundant.
Again, my comments about the unused parameter, always using
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 memberClass
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 memberContext
StackExchange Code Review Q#621, answer score: 16
Revisions (0)
No revisions yet.