«

Do you have some suggestions for improvement. I have just written the half
of the program yet. Will continue the next half after suggestions for
improvement:



/* A program to find the Income Tax of an individual as per Indian Tax
Slabs:

Yearly Income slab (Rs.) Tax Rate (%)
Up to 1,50,000 NIL
Up to 1,80,000 (for women) NIL
Up to 2,25,000 NIL
(for resident individual of 65 years or above)

1,50,001 - 300,000 10
3,00,001 - 500,000 20
5,00,001 upwards 30

A surcharge of 10 per cent of the total tax liability is applicable where the total income exceeds Rs 1,000,000.
*
* DESIGN: 1st, we will write code to get information of the user
* 2nd, we calculate the Tax.
*
*
* VERSION 1.0
*
*/


#include <iostream>
#include <vector>
#include <string>
#include <limits>
#include <cfloat>

void get_info( long double&, int&, std::string& );

void get_payee_income( long double& );
void get_payee_age( int& );
void get_payee_sex( std::string& );


int main()
{
// const int ms = 150000;
// const int ws = 180000;
// const int ss = 225000;
long double income;
int age;
std::string sex;

get_info(income, age, sex);

std::cout << "your age = "
<< age
<< ", Your Sex = "
<< sex
<< ", Your income = "
<< income
<< std::endl;

//calculate_tax();

return 0;
}




void get_info(long double& i, int& a, std::string& s)
{
get_payee_income( i );
get_payee_age( a );
get_payee_sex( s );
}



void get_payee_income( long double& x )
{
const int min_income = 0;
const long double max_income = ULONG_MAX;

/* a combination of long double and ULONG_MAX is a catch, because if you put "unsigned long int max_income" on Left hand side
and user inputs value larger then ULONG_MAX then program, automagically will convert (cast ?) the value to some lower value
to satify the Left Hand Side and that will make it behave strange in some ways, doing some information loss. So I used
the maximum I have got, long double.

Is this the correct solution ? .. I don't think so.. I don't want to print the income in e+06 format. It needs to be a number.
*/

while ( (std::cout << "Enter your income: ") &&
(!(std::cin >> x)) ||
( x < min_income ) ||
( x > max_income ))
{
std::cin.clear();
std::cin.ignore(std::numeric_limits<std::streamsiz e>::max(), '\n');
std::cout << "Don't play games. Enter a valid figure\n";
}
}



/* get age of the user, I have made sure it can handle stupid users */
void get_payee_age( int& age )
{
const int calments_age = 122;
const int min_age_for_work = 18;

while( ( std::cout << "Please enter your age: ") &&
(!(std::cin >> age)) ||
(age >= calments_age) ||
( age < min_age_for_work ) )
{

std::cout << "1- Oldest man lived on this Earth was Jeanne Calment of France (1875-1997)\n"
<< "who died at age 122 years and 164 days\n\n"
<< "2- Minimum age of work is "
<< min_age_for_work
<< "\n\n"
<< "3- Nthign other than numbers is accepted\n\n";
std::cin.clear();
std::cin.ignore(std::numeric_limits<std::streamsiz e>::max(), '\n');
}
}



/* get whether the user is a man or woman */
void get_payee_sex( std::string& t )
{

std::cout << "Please enter whether you are a man or a woman (only in small letters please): ";
std::cin >> t;

const std::string m = "man";
const std::string w = "woman";

int condm = t.compare(m);
int condw = t.compare(w);

while( condm )
{
if( 0 == condw ) break;

std::cout << "You must be either man or woman.I hope you have mistyped. Try Aagin: ";
std::cin >> t;

condm = t.compare(m);
condw = t.compare(w);
}
}




--
www.lispmachine.wordpress.com
my email is @ the above blog.

arnuld wrote:


I'm assuming that you're only doing real persons and not things like US
corporations or whatever the equivalent would be there.

class Person { // not a great name
// I leave what Money is to you, but floating types
// can be problematic for many uses of this kind.
Money income;
// I suspect it's much better to store a birthdate instead of
// an age.
Date birthdate;
// std::string doesn't strike me as a good choice for this
Sex sex;
..
..
..
};


> void get_info( long double&, int&, std::string& );
>
> void get_payee_income( long double& );
> void get_payee_age( int& );
> void get_payee_sex( std::string& );
>

Consider making a ctor, or a function that returns a Person


>
> int main()
> {
> // const int ms = 150000;
> // const int ws = 180000;
> // const int ss = 225000;
> long double income;
> int age;
> std::string sex;
>
//> get_info(income, age, sex);

const Person person = get_info(); // I think this is better.

// and write something that will allow you to
std::cout << person << std::endl;

> std::cout << "your age = "
> << age
> << ", Your Sex = "
> << sex
> << ", Your income = "
> << income
> << std::endl;
>
> //calculate_tax();

person.tax(); // should return Money


>
> return 0;
> }
>
>
>
>

This is your second get_info.

> void get_info(long double& i, int& a, std::string& s)
> {
> get_payee_income( i );
> get_payee_age( a );
> get_payee_sex( s );
> }
>
>
>



I would prefer
Money get_payee_income() {...
or even
double get_payee_income() {...

> void get_payee_income( long double& x )



> {

Why not just make these double?
const long double min_income = 0.0;
const long double max_income =
std::numeric_limits<long double>::max();

> const int min_income = 0;
> const long double max_income = ULONG_MAX;

Why not:
const long double max_income = std::numeric_limits<long>::max();


>
> /* a combination of long double and ULONG_MAX is a catch, because if you put "unsigned long int max_income" on Left hand side
> and user inputs value larger then ULONG_MAX then program, automagically will convert (cast ?) the value to some lower value
> to satify the Left Hand Side and that will make it behave strange in some ways, doing some information loss. So I used
> the maximum I have got, long double.
>
> Is this the correct solution ? .. I don't think so.. I don't want to print the income in e+06 format. It needs to be a number.
> */
>


I think this loop is hard to read.


> while ( (std::cout << "Enter your income: ") &&
> (!(std::cin >> x)) ||
> ( x < min_income ) ||
> ( x > max_income ))

consider implementing two template functions,

template<typename T>
bool isInInclusiveRange(const T &low, const T &value, const T &high) {
// maybe better to write these with only the < operator
return low <= value && value <= high;
}

and a similar one for isInExclusiveRange



> {
> std::cin.clear();
> std::cin.ignore(std::numeric_limits<std::streamsiz e>::max(), '\n');
> std::cout << "Don't play games. Enter a valid figure\n";

(I put all that on multiple lines to avoid wrapping problems.)
I'm not commenting here on the logic of your i/o, but on the issue of
defining constants and when to use them. You know what the min and max
values are, you should let your users know too. Similarly for sex below.
std::cout << "Don't play games. "
<< "Enter a valid figure, between "
<< min_income
<< " and "
<< max_income
<< "." << std::end;


> }
> }
>
>
>
> /* get age of the user, I have made sure it can handle stupid users */
> void get_payee_age( int& age )
> {

Or
int get_payee_age() { ...


I think that naming a variable this way is not a good idea. It is
amusing though.
> const int calments_age = 122;


const int max_age = 122;
> const int min_age_for_work = 18;
>
> while( ( std::cout << "Please enter your age: ") &&
> (!(std::cin >> age)) ||
> (age >= calments_age) ||
> ( age < min_age_for_work ) )
> {

I suspect that the message below will require maintenance. Useful
programs spend most of their time in maintenance, it's not generally a
good idea to do something that will add to that. Sadly, I suspect that
my name will not appear in the message below, but it's likely, if your
program is useful, that eventually someone else's name will.

Wasn't Jeanne Calment of France a woman?



> std::cout << "1- Oldest man lived on this Earth was Jeanne Calment of France (1875-1997)\n"
> << "who died at age 122 years and 164 days\n\n"
> << "2- Minimum age of work is "
> << min_age_for_work
> << "\n\n"
> << "3- Nthign other than numbers is accepted\n\n";
> std::cin.clear();
> std::cin.ignore(std::numeric_limits<std::streamsiz e>::max(), '\n');
> }
> }
>
>
>

Just to beat this thing up again, I'd prefer
std::string get_payee_sex() { ...

> /* get whether the user is a man or woman */
> void get_payee_sex( std::string& t )
> {
>
> std::cout << "Please enter whether you are a man or a woman (only in small letters please): ";
> std::cin >> t;
>
> const std::string m = "man";
> const std::string w = "woman";

If the constants should ever change, then it's easier to not have to
change the messages too.
std::cout << "Please enter whether you are a " << m << " or a "
<< w << " (only in small letters please): ";


>
> int condm = t.compare(m);
> int condw = t.compare(w);
>
> while( condm )
> {
> if( 0 == condw ) break;
>
> std::cout << "You must be either man or woman.I hope you have mistyped. Try Aagin: ";
> std::cin >> t;
>
> condm = t.compare(m);
> condw = t.compare(w);
> }
> }

while(t != m && t != w) {
std::cout << " your error message here "
std::cin >> t;
}

HTH.

LR

> On Mon, 05 Jan 2009 12:16:13 -0500, LR wrote:

> I'm assuming that you're only doing real persons and not things like US
> corporations or whatever the equivalent would be there.

Right, for general middle class Indian. Thats why I wanted to stick with
the values less than long integer, as five digits is the rare maximum of
what an Indian middle class man gets.




> class Person { // not a great name
> // I leave what Money is to you, but floating types
> // can be problematic for many uses of this kind.
> Money income;
> // I suspect it's much better to store a birthdate instead of
> // an age.
> Date birthdate;
> // std::string doesn't strike me as a good choice for this
> Sex sex;

I really wonder why you bring out the Class here, if I do it by the
procedural Style, will it be fine or using Class gives me some advantage ?

2nd, If I use date of birth, not age, then I have to add some more lines
of code, first to fetch the original value like Jan 24th, 1980 and then
converting it to integer (1980 in our case), it will add one more function
(or member function) increasing the complexity of the code.


> Consider making a ctor, or a function that returns a Person

you mean a constructor (ctor) ?

I have never ever done any OOP, heck I even don't know how to write a
class, I learned C++ first and then C but these days all I am *able* to
think is of procedures/functions and not of std::istream or <template>.
Seems like as if I never learned them





> Why not just make these double?
> const long double min_income = 0.0;
> const long double max_income =
> std::numeric_limits<long double>::max();

actually using double (or long double) prints income in e+ format which is
not what I want. A readable number is desired like 213476.


> I think this loop is hard to read.

it is ..



> consider implementing two template functions,
>
> template<typename T>
> bool isInInclusiveRange(const T &low, const T &value, const T &high) {
> // maybe better to write these with only the < operator return low
> <= value && value <= high;
> }
> }
> and a similar one for isInExclusiveRange


okay, I will learn how to write a template but first how to even read one.
this will be the first template of my life




> Wasn't Jeanne Calment of France a woman?

Eh.. my mistake. I got some very strange information about her from
Wikipedia: At age 85, she took up fencing, and at 100, she was still
riding a bicycle. She gave up smoking only five years before her death. On
the top of that she lived on her own till she was 110 years of age.






--
www.lispmachine.wordpress.com
my email is @ the above blog.

arnuld wrote:
>> On Mon, 05 Jan 2009 12:16:13 -0500, LR wrote:
>
>> I'm assuming that you're only doing real persons and not things like US
>> corporations or whatever the equivalent would be there.
>
> Right, for general middle class Indian. Thats why I wanted to stick with
> the values less than long integer, as five digits is the rare maximum of
> what an Indian middle class man gets.

But someone might use your code who doesn't fit in this category,
perhaps even to calculate the tax owed by someone else, an employee
perhaps. Or someone might like your code so much they'll want you to
rewrite it to handle corporations or limited partnerships or whatnot. Be
prepared.

But maybe you're right. Get the simpler case to work first, and then
build on it.


>> class Person { // not a great name
>> // I leave what Money is to you, but floating types
>> // can be problematic for many uses of this kind.
>> Money income;
>> // I suspect it's much better to store a birthdate instead of
>> // an age.
>> Date birthdate;
>> // std::string doesn't strike me as a good choice for this
>> Sex sex;
>
> I really wonder why you bring out the Class here, if I do it by the
> procedural Style, will it be fine or using Class gives me some advantage ?

Of course you can write your code with out classes. But IMHO easier to
write this with classes. At the very least makes it easier to pass the
data around.

>
> 2nd, If I use date of birth, not age, then I have to add some more lines
> of code, first to fetch the original value like Jan 24th, 1980 and then
> converting it to integer (1980 in our case), it will add one more function
> (or member function) increasing the complexity of the code.


I don't know how taxation works in India. But I'll make some
assumptions. Consider your user, you ask their age, they enter it, and
you do the taxes. But they may not remember that they're entering the
wrong age for the year they were interested in. Suppose they want to
calculate their taxes for fiscal 1998, or 2014? I think it's much nicer
for the computer to do that work.


>> Consider making a ctor, or a function that returns a Person
>
> you mean a constructor (ctor) ?

Yes.

>
> I have never ever done any OOP, heck I even don't know how to write a
> class, I learned C++ first and then C but these days all I am *able* to
> think is of procedures/functions and not of std::istream or <template>.
> Seems like as if I never learned them

No time like the present. It's easier to get started then you think.
You don't have to worry too much about getting everything all at once,
and I think this type of problem offers a nice trade off, not too easy,
not too complicated.

Go ahead. Just write one little class.




>> Why not just make these double?
>> const long double min_income = 0.0;
>> const long double max_income =
>> std::numeric_limits<long double>::max();
>
> actually using double (or long double) prints income in e+ format which is
> not what I want. A readable number is desired like 213476.

Then print them that way. You can control the format. Look up the
<iomanip> and <ios> headers and for things like std::setprecision and
std::fixed etc.

But creating a Money class might help with this. You may want to look
at the <locale> header for info on things like punctuation and io. But
if that's too much, then even just making a simple class like:

class Money {
double amount;
public:
Money();
Money(const Money &);
Money &operator=(const Money &);
.
. // other useful ctors and member functions are left to you
.
friend std:stream &operator<<(std:stream &, const Money &);
};

will be useful. But be careful if you need to calculate paise. Those
doubles can be pretty tricky. OTOH, if you decide that double isn't what
you want, then changing the type of amount to something else shouldn't
be too hard. Easier than going through all of your code and changing
individual variables.

Just consider yourself lucky that you're not doing this for rupees,
annas and pice.



>> I think this loop is hard to read.
>
> it is ..

IMO, writing code is a communicative art. Even if you're just writing
it for yourself. Names of variables and constants should communicate
what their purpose is. Things like conditions in loops should be easy
to read and understand. Someone, maybe you, will have to maintain your
code, striving for readability isn't a bad idea.





>> consider implementing two template functions,
>>
>> template<typename T>
>> bool isInInclusiveRange(const T &low, const T &value, const T &high) {
>> // maybe better to write these with only the < operator return low
>> <= value && value <= high;
>> }
>> }
>> and a similar one for isInExclusiveRange
>
>
> okay, I will learn how to write a template but first how to even read one.
> this will be the first template of my life

If this is your first template, then write this function first, again I
formatted things this way to avoid line wrapping (untested):

bool inInclusiveRange(
const double &low,
const double &value,
const double &high
) {
return low <= value && value <= high;
}

then turn it into a template, if you need to.

LR

okay, This is what I have came up with yet:


#include <iostream>
#include <vector>
#include <string>
#include <limits>
#include <cfloat>

typedef long int Income;
typedef int Age;
typedef std::string Sex;

void get_info();
bool if_in_range( const long int, const long int, const long int );
Income get_payee_income();
Age get_payee_age();
Sex get_payee_sex();



class Person
{
Income income;
Age age;
Sex sex;

public:
Person(); /* This is constructor declaration, we will define it
later, don't put code of functions into the class */
/* ~Person(); don't know whow to write a destructor yet */
void tax() const; /* will print the calculated Tax of the object */
void person() const; /* will print the whole information of the
object: income, age and sex */
};



/* constructor */
Person:erson()
{
income = get_payee_income();
age = get_payee_age();
sex = get_payee_sex();
}


/* will calculate and print the Tax of the object */
void Person::tax() const
{
/* calculate Tax here */
}


/* print the information about the object */
void Person:erson() const
{
std::cout << "Income: " << income << '\n'
<< "Age: " << age << '\n'
<< "Sex: " << sex << std::endl;
}




int main()
{
const Person person;

person.person();

return 0;
}



/* This function can not handle a little larger inputs. It goes into
an infinite loop
for larger values like 1234567890123. Can we fix it ? */
Income get_payee_income()
{
Income x;
const long int min_income = 0;
const long int max_income = LONG_MAX ; /* using
std::numeric_limits<int>::max; gives error */

/* because of the above line, the program falls into infinite
"while" loop */
while ( (std::cout << "Enter your income: ") && (!(std::cin >> x))
&&
(if_in_range( min_income, x, max_income )) )
{
std::cin.clear();
std::cin.ignore(std::numeric_limits<std::streamsiz e>::max(), '\n');
std::cout << "Don't play games. Enter a valid figure\n";
}


return x;
}




/* get age of the user, I have made sure it can handle stupid users */
Age get_payee_age()
{
Age age;
const int calments_age = 122; /* Jeanne Calment is the oldest woman
ever lived */
const int min_age_for_work = 18;

while( (std::cout << "Please enter your age: ") && (!(std::cin >>
age)) &&
if_in_range( min_age_for_work, age, calments_age ))
/* In if_in_range() function implicit conversion of arguments from
int to long int
will not cause any information loss here. Just don't worry
aboout it */
{

std::cout << "Unacceptable Value, your age must be between "
<< min_age_for_work
<< " - "
<< calments_age
<< "\n\n"
<< "Nothign other than numbers is accepted\n\n";
std::cin.clear();
std::cin.ignore(std::numeric_limits<std::streamsiz e>::max(),
'\n');
}

return age;
}



/* get whether the user is a man or woman */
Sex get_payee_sex()
{
Sex t;
std::cout << "Please enter whether you are a man or a woman (only in
small letters please): ";
std::cin >> t;

const std::string m = "man";
const std::string w = "woman";

int condm = t.compare(m);
int condw = t.compare(w);

while( condm )
{
if( 0 == condw ) break;

std::cout << "You must be either man or woman.I hope you have
mistyped. Try Aagin: ";
std::cin >> t;

condm = t.compare(m);
condw = t.compare(w);
}

return t;
}





/* ------------------- Small Helper functions ----------------- */

/* check whether a long int value lies between 2 values */
bool if_in_range( const long int low, const long int val, const long
int high )
{
return (val >= low && val <= high);
}

arnuld schrieb:
> okay, This is what I have came up with yet:

Some comments inline:

> #include <iostream>
> #include <vector>
> #include <string>
> #include <limits>
> #include <cfloat>
>
> typedef long int Income;
> typedef int Age;
> typedef std::string Sex;
>
> void get_info();
> bool if_in_range( const long int, const long int, const long int );

If you make this a template, you can pass every type to this function,
even unsigned or double.

Also, provide function parameter names. With this declaration, nobody
knows which parameter is what. I'd call it so:
if_in_range(value, min, max)
But the usage below is different.

The name is misleading, too. With "if_*", you except it to do something
with side-effects. A function that only checks and returns a bool would
be better named is_in_range.

> Income get_payee_income();
> Age get_payee_age();
> Sex get_payee_sex();
>
>
>
> class Person
> {
> Income income;
> Age age;
> Sex sex;
>
> public:
> Person(); /* This is constructor declaration, we will define it
> later, don't put code of functions into the class */
> /* ~Person(); don't know whow to write a destructor yet */

You don't need a destructor. The compiler supplied is good enough.

> void tax() const; /* will print the calculated Tax of the object */
> void person() const; /* will print the whole information of the
> object: income, age and sex */
> };
>
>
>
> /* constructor */
> Person:erson()
> {
> income = get_payee_income();
> age = get_payee_age();
> sex = get_payee_sex();
> }

Initialize, don't assign:

Person:erson()
: income(get_payee_income(),
age(get_payee_age()),
sec(get_payee_sex())
{}

> int main()
> {
> const Person person;
>
> person.person();

This line demonstrates, that the member function "person" is misnamed

> return 0;
> }
>
>
>
> /* This function can not handle a little larger inputs. It goes into
> an infinite loop
> for larger values like 1234567890123. Can we fix it ? */
> Income get_payee_income()
> {
> Income x;
> const long int min_income = 0;
> const long int max_income = LONG_MAX ; /* using
> std::numeric_limits<int>::max; gives error */

max is a function. You have to call it.

> /* because of the above line, the program falls into infinite
> "while" loop */
> while ( (std::cout << "Enter your income: ") && (!(std::cin >> x))
> &&
> (if_in_range( min_income, x, max_income )) )
> {

If you don't want to check the output operation for success, you can use
the comma operator:
while (std::cout << "output", !(std::cin >> x)) { ... }

Second, the logic is wrong. You want to stop the loop if the entered
value is in the range, don't you? Correct would be:

while (std::cout << "Enter your income: ",
!(std::cin >> x) || !if_in_range( min_income, x, max_income ))
{ /* ... */ }

> std::cin.clear();
> std::cin.ignore(std::numeric_limits<std::streamsiz e>::max(), '\n');
> std::cout << "Don't play games. Enter a valid figure\n";
> }

--
Thomas

arnuld wrote:
> okay, This is what I have came up with yet:
>

There is a lot to say about your program. Most of which is primarily
design related.

>[snip]
>
> typedef long int Income;
> typedef int Age;
> typedef std::string Sex;

I would have made those classes. It localizes information about types
such as limits or validity checking and hide representation.

struct Income
{
typedef long int value_type;
//code handling value/setting if needed


static const long int min_value = 0;
static const long int max_value = LONG_MAX ;

static bool is_valid(value_type v)
{
return if_in_range( min_value, x, max_value );
}
};

Same for age and sex. I would make Sex an enum.


> void get_info();

this is not used.

> bool if_in_range( const long int, const long int, const long int );

You didn't provide name parameter and I had to look for the
implementation to see how to use it.
Personally, I would prefer something like:

template<typename T=long>
struct range
{
range(T low, T high):low(low),high(high){}
T low;
T high;

bool contains(T value)const
{
return value >= low && value <= high;
}
};

And use:
range(0,4242).contains(42);

> Income get_payee_income();
> Age get_payee_age();
> Sex get_payee_sex();
>
>
>
> class Person
> {
> Income income;
> Age age;
> Sex sex;
>
> public:
> Person(); /* This is constructor declaration, we will define it
> later, don't put code of functions into the class */
> /* ~Person(); don't know whow to write a destructor yet */
> void tax() const; /* will print the calculated Tax of the object */
> void person() const; /* will print the whole information of the
> object: income, age and sex */
> };

Make tax() and person() free functions.
If they diplay information, it should be included in their name and they
should take the stream to use in parameter (for parametrization from above):
display_tax(ostream& os);
display_information(ostream& os);



> /* constructor */
> Person:erson()
> {
> income = get_payee_income();
> age = get_payee_age();
> sex = get_payee_sex();
> }

This is not good design. The Person class shouldn't be responsible for
collecting the information. Make a dumb constructor and call:

Person person(
get_payee_income(),
get_payee_age(),
get_payee_sex()
);

Best is:
Person get_payee()
{
return Person(
get_payee_income(),
get_payee_age(),
get_payee_sex()
);
};

And hide get-payee_*().

[snip]
> /* This function can not handle a little larger inputs. It goes into
> an infinite loop
> for larger values like 1234567890123. Can we fix it ? */
> Income get_payee_income()
> {
Income::value_type x;
do
{
std::cout << "Enter your income: ";
if(std::cin >>x) && Income::is_valid(x) )
{
return x;
}
std::cin.clear();
std::cin.ignore(std::numeric_limits<std::streamsiz e>::max(),'\n');
std::cout << "Don't play games. Enter a valid figure\n";
}while(!std::cin.eof());

//handle error of input here
//CTL-D typed
throw std::runtime_error("wrong input");

//never reach this line but keep compiler happy
return -1;
}

Same for other inputs.



--
Michael