Help on OO leap year calculator

[Protoman]

Hi!!! I need some help on a project I'm that calculates leap years; I'm
getting errors that I have no idea what they mean; here's the code:

#include <iostream>
#include <cstdlib>
using namespace std;

class Year
{
public:
Year(int y=0):year(new int) *year(y){}
~Year(){delete year;}
Year(const Year& rhs)*year(rhs.*year){}
Year operator=(const Year& rhs)
{
if(this!=&rhs)
{
*year=rhs->year;
delete year;
}
else return this;
}
void SetYear(int y):year(new int),*year(y){}
int GetYear()const{return *year;}
bool IsLeapYear(const Year& y)const
{
if(y->year%400==0&&y->year%4==0)
return true;
else if(y->year%100)
return false;
else return false
}
friend ostream& operator<<(ostream& os,const Year& y);
private:
int* year;
};

ostream& operator<<(ostream& os,const Year& y)
{
os << y.GetYear();
return os;
}
}
int main()
{
int date;
for(;
{
cout << "Enter a year: " << endl;
cin >> date;
Year year(date);
if(year.IsLeapYear)
cout << year << " is a leap year. " << endl;
else
cout << year << " is not a leap year. " << endl;
bool cont;
cout << "Continue?[1=yes|0=no]: " << endl;
cin >> cont;
if(cont==true)continue;else break;
system("PAUSE");
return 0;
}


And as a side note, it's not letting me use constructor inizilization
syntax. Can you help me (this isn't homework)? Thanks for your help!!!

 

[Daw]

And what erros u get?

 

[Daw]

And I'm sure this _is_ a school work ;>

 

[Protoman]

No it's not; my friend, Jonathon Mastron at BIOLA, asked me to it for
him. My school work is trig stuff, not calendars and dates. Just help
me please.

 

[Protoman]

OK, I fixed it; it compiles and runs now; here's the code:

#include <iostream>
#include <cstdlib>
using namespace std;

class Year
{
public:
Year(int y=0): year(y){}
~Year(){}
Year(const Year& rhs):year(rhs.year){}
Year operator=(const Year& rhs)
{
if(this!=&rhs)
year=rhs.year;
return *this;
}
void SetYear(int y){year=y;}
int GetYear(void)const{return year;}
bool IsLeapYear(void)const
{
if(year%400==0&&year%4==0)
return true;
else if(year%100)
return false;
else return false;
}
friend ostream& operator<<(ostream& os,const Year& y);
private:
int year;
};

ostream& operator<<(ostream& os,const Year& y)
{
os << y.GetYear();
return os;
}

int main()
{
int date;
for(;
{
cout << "Enter a year: " << endl;
cin >> date;
Year year(date);
if(year.IsLeapYear())
cout << year << " is a leap year. " << endl;
else
cout << year << " is not a leap year. " << endl;
bool cont;
cout << "Continue?[1=yes|0=no]: " << endl;
cin >> cont;
if(cont==true)continue;else break;
}
system("PAUSE");
return 0;
}

Anyway I can make it more efficient? And other than calculating leap
years, what other uses can I have for this class? And what other ops
should I overload? Thanks for the help!!!

 

[Protoman]

I'm trying to overload operator>>, but my program keeps aborting;
here's the code for operator>>:

istream& operator>>(istream& is,const Year& y)
{
is >> y.year;
return is;
}

Why does it keep aborting?

 

[Raymond Martineau]

I'm trying to overload operator>>, but my program keeps aborting;
here's the code for operator>>:

istream& operator>>(istream& is,const Year& y)
{
is >> y.year;

You are trying to modify a const value. In fact, the const qualifier is
not appropriate for this kind of function.

return is;
}

Why does it keep aborting?

If it aborts at that region, then perhaps there might be a logic error in
your code. Best to find some form of debugger.

 

[Protoman]

I fixed it and it works.

 

[benben]

to encapsulate, you should really make the leap year identification a
non-friend, non-member, standalone function.

like:

bool is_leapyear(Year year){/*...*/}

ben

 

[Greg]

OK, I fixed it; it compiles and runs now; here's the code:
..
bool IsLeapYear(void)const
{
if(year%400==0&&year%4==0)
return true;
else if(year%100)
return false;
else return false;
}

This function is clearly not correct. It will return false for nearly
every year that is a leap year. The first clue that something is amiss,
is the fact that it tests whether a year is divisible by 4 after it
knows it is divisible by 400. Anyway, here is a corrected version:

bool IsLeapYear() const
{
if ( year%4 ) // not divisible by 4
return false;
if ( year%100 ) // not a century year
return true;
if ( year%400 ) // not a fourth century
return false;
return true;
}

Also, I would work on writing more readable code. The layout of the
code in the original program is atrocious. Do spaces cost extra?

Greg

 

[Howard]

to encapsulate, you should really make the leap year identification a
non-friend, non-member, standalone function.

like:

bool is_leapyear(Year year){/*...*/}

ben

How does that help anything? How does it provide encapsulation, when it's
now an external function which has to know (and have access to) the
internals of the Year class?

And the year member of that object is a private variable, so if this is a
non-friend, it can't possibly access the member.

(Also, you're passing the Year object by value, which requires a copy for no
apparent reason.)

-Howard

 

[Kai-Uwe Bux]

How does that help anything?

Helps to keep the interface of Year small. That makes for fewer things to
care about when you actually try to reap the advantages of encapsulation
and set out to change the implementation. You will not need to even look at
is_leapyear().

How does it provide encapsulation, when it's
now an external function which has to know (and have access to) the
internals of the Year class?

Maybe I am imagination impaired, but why would is_leapyear() need to know
"the internals" of Year. Don't you think that an ordinary call to a public
member function like

signed long GetYear() const;

would suffice?

And the year member of that object is a private variable, so if this is a
non-friend, it can't possibly access the member.

So you decided that a GetYear() function is inherently bad and that your
Year class will never ever allow any client to actually figure out which
year a given object of type Year represents. That, indeed, is very tight
encapsulation.

Admittedly, I have a hard time imagining a Year class to have interesting
internals (the only invariant I can fathom is that the year cannot be 0).
However, dreaming up an interface of Year that would not allow non-friend
clients to tell whether the year is divisible by 4 and by 400 is beyond me.

(Also, you're passing the Year object by value, which requires a copy for
no apparent reason.)

Good point. However, what would you think that sizeof(Year) should be?


Best

Kai-Uwe Bux

 

[wittempj]

Your IsLeapYear method is wrong, e.g. 2004 will fail: 2004%400 == 4,
2004%4 == 0, so the first if will fail, 2004%100 == 4, so the else if
will fail, so the method will return false.

see http://www.faqs.org/faqs/calendars/faq/part1/ for the correct
method.

 

[Howard]

Howard wrote:


Admittedly, I have a hard time imagining a Year class to have interesting
internals (the only invariant I can fathom is that the year cannot be 0).
However, dreaming up an interface of Year that would not allow non-friend
clients to tell whether the year is divisible by 4 and by 400 is beyond
me.

Looking more at his class, I'd have to agree. I see no reason for the class
in the first place, really. A year is simply an integer. Why does it need
functionality? Making the internal value a pointer to an integer is even
more needless complexity.

-Howard

 

[int2str]

I fixed it and it works.

C++ language issues aside, your leap year calculation is flawed. For
example, according to your code, 1996 is not a leap year.

Also, I'm not sure if a complete class isn't a little overkill, unless
you do more with the year than figuring out if it's a leap year.

This formula should work better:

bool IsLeapYearFixed( const unsigned int year )
{
if ( 0 != year % 4 )
return false;

if ( 0 == year % 400 )
return true;

if ( 0 == year % 100 )
return false;

return true;
}

Cheers,
Andre