Code Style Review

[Reewr]

My question is quite easy to answer for those of you who have done some programming, however, as a beginner I can't really answer it that easily myself.

Is my code style readable and/or is it an optimal way of doing what I was ment to do.

This is a very easy program that is ment to determine wether the year that a user inputs is a leap year or not.

#include <iostream>

using namespace std;

int InputValue();
void CheckIfLeapYear(int x);

int main()
{
	int nReturnYear = InputValue();
	CheckIfLeapYear(nReturnYear);

}

int InputValue()
{
	int nYear;
	cout << "Please enter a year" << endl;
	cin >> nYear;
	return nYear;
}

void CheckIfLeapYear(int x)
{
	if(x%4 == 0 && x%100 != 0 || x%4 == 0 && x%400 == 0)
		cout << x << " is a leap year" << endl;
	else
		cout << x << " is not a leap year" << endl;
}

The program is working as intended, however, I'm not sure about the if statement, wether that can be rewritten to be more readable or better coded in anyway.

I would love to hear your thoughts.

Thanks...!

[Zhuge]

You could reduce it a bit to just say x%4 == 0 && (x%100 != 0 || x%400 == 0).

I would also personally make the leap year checker just return a boolean value as to whether or not the supplied year is a leap year. That way, you can do more with the result than just print whether it is the case or not.

[MrHutch]

Looks pretty readable to me.

Two slight suggestions:
1) In your if statement, maybe include some brackets to group expressions together. It helps tidy it up and clarify what you want the statement to do.

2) Add comments. The easiest way to convey what you were trying to do to another programmer is by making sure your code as clear and concise comments in.

EDIT: 3) Yeah, I'd also go with Zhuge's suggestion of a boolean returning function to indicate whether it's a leap year or not.

4) Also, maybe some validation on the user input?

[TheIdeasMan]

My 1 cent worth:

main is supposed to return an int, so put return 0; before the closing brace of main

[Zhuge]

main() will actually implicitly return 0; at its end even if you don't specify it, so it isn't required.

[Catfish2]

main is supposed to return an int, so put return 0; before the closing brace of main

Wrong.
http://stackoverflow.com/questions/22239/why-does-int-main-compile

@ Reewr:
It looks pretty good to me.
I would, however, refrain from using namespace std;, and I would leave a free line between variable declarations and code -- but that's up to you.

[Reewr]

Thanks for all your help on this. I didn't really expect so many replies this quickly.

I've added your suggestions and come up with this..

bool CheckIfLeapYear(int x)
{
	bool bLeapYear;

	if(x%4 == 0 && (x%100 != 0 || x%400 == 0)) //if dividable by 4 and not by 100 or dividable by 400 = leap year
	{
		bLeapYear = 1; //returns true if it's a leap year

		return bLeapYear;
	}
	else
	{
		bLeapYear = 0; //returns false if it's not a leap year

		return bLeapYear;
	}
}

Was this something you guys had in mind? (also changed main etc.)

Edit:When it comes to comments though, how long should they be at maximum..? I feel like my current one becomes a bit too long.

@iHutch105:
When it comes validation, what exactly did you have in mind? Something that checks if the year is actually correct and that it contains numbers etc?

@Catfish2:
Why would you refrain from using using namespace std;?
I just feel it looks a bit more messy when you have to add std:: everywhere, but that might just be me..


If my english is a bit dodgy it's because it's not my first language.

[MrHutch]

Yeah, exactly. Something that only accepts 4 digit, valid dates.

Some I can't enter stuff like:
748295
-1994
ab48
1
19!5

[Catfish2]

Comment theory time!
1) Your code should comment itself: this is done by using good names for variables, functions, and data types. You pretty much got this right.
2) Where the code is obscure (this is a bit subjective), you comment the intent. This also means you do not comment trivialities, which are easy to understand (again, subjective).

So in your code above, I think the last two comments are a bit useless.
You also use 0 and 1 instead of true and false, as in bLeapYear = false;.

Edit:
And I noticed you use a useless temporary. You could simplify your code:

bool CheckIfLeapYear(int year)
{
    if (year % 4 == 0 && (year % 100 != 0 || year % 400 == 0))
        return true;

    return false;
}

A return exits the function anyway, so an else branch isn't strictly needed.

[Reewr]

@iHutch105:
Exactly what I thought. I might add a loop that asks for a new number if the input number is not accepted.

@Catfish2:
Yeah, I thought the two ones were pretty self-explanatory. Yeah, I forgot about that when I edited the code to return something.. :p

Thanks to all of you who helped, I really appreciate it.

I'll mark this as resolved now as I have all my questions answered. I'm pretty sure you'll see me again though, as I've never actually stumbled upon a forum that's as helpful as you..!

[vlad from moscow]

I would write something as

#include <iostream>

unsigned GetYear();
inline bool IsLeapYear( unsigned year );

int main()
{
	unsigned year = GetYear();

	if ( IsLeapYear( year ) )
	{
		std::cout << year << " is a leap year"
		          << std::endl;
	}
	else
	{
		std::cout << year << " is not a leap year"
		          << std::endl;
	}

	return 0;
}

unsigned GetYear()
{
	unsigned year;

	std::cout << "Please enter a year"
	          << std::endl;
	std::cin >> year;

	return year;
}

bool IsLeapYear( unsigned year )
{
	return  ( year % 4 == 0  ) && 
                ( year % 100 != 0 || year % 400 == 0 );
} 

[Reewr]

It looks a lot more simplified. And I didn't think of Unsigned at all as I've been working mostly with Java.

Thanks Vlad, I'll look through it and see what I can get used to from what you've posted.

[TheIdeasMan]

main() will actually implicitly return 0; at its end even if you don't specify it, so it isn't required.

Well, the things you learn !! I have been doing that since 1987.

[Catfish2]

You're on Vlad!

#include <iostream>
#include <limits>

unsigned int getYear()
{
    const unsigned int yearMin = 100;
    const unsigned int yearMax = 2020;
    unsigned int r;

    std::cout << "Please enter a year: ";

    while (!(std::cin >> r) || r < yearMin || r > yearMax)
    {
        std::cout << "Please enter a VALID year: ";
        std::cin.clear();
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
    }

    return r;
}

bool isLeapYear(unsigned int year)
{
    return year % 4 == 0 && (year % 100 != 0 || year % 400 == 0);
}

int main()
{
    unsigned int year = getYear();

    std::cout << year << (isLeapYear(year) ? " is " : " is not ") << "a leap year.";
    std::cout << std::endl;
}

Although this thing still fails gloriously if you enter something like 123abc.
I guess the real solution is to use C++11's regex library.

@ TheIdeasMan: From what I know, you only don't need to since C99 and C++98. So probably in 1987, you did.

[MrHutch]

Although this thing still fails gloriously if you enter something like 123abc.

You sure? It seemed to work fine for me.

One thing I will say is that the Gregorian calendar started in 1582, so that would be a more realistic yearMin value.

And, since this is original a thread about coding style, I tend to favour all upper case characters for constant names.

[Catfish2]

You sure? It seemed to work fine for me.

In my tests, it cuts out 123 from 123abc, which it shouldn't do.

And, since this is original a thread about coding style, I tend to favour all upper case characters for constant names.

That's a good idea to be considered!
I only don't do that because I'm used to reserve such style for macros instead.

[zb0S216C]

Reewr wrote:

void CheckIfLeapYear(int x);

I would've returned a bool here instead of printing the result. Also, x could've been named in a way that's more descriptive, such as date. x could've been constant since its value never changes throughout the duration of the function. Constants are efficient. Finally, x could be unsigned since dates are never negative.

Reewr wrote:

cin >> nYear;
return nYear;

You're not checking for invalid input. What if somebody entered a string?

Reewr wrote:

if(x%4 == 0 && x%100 != 0 || x%4 == 0 && x%400 == 0)

This could be miles better. Use parentheses and spaces here; it's hard to read. In fact, a ternary operator could actually be more efficient (if you keep the std::cout statements within the function), and therefore, the function could be in-lined more effectively; thus, using the CPU cache effectively (more hits).

Too harsh?

_Wazzak

[Reewr]

Loads of comments since I last checked, so it took me quite a while to check through them all.

@Framework:

Not too harsh at all. I didn't actually know that you could name them something else as I've seen (int x, int y, int z) used all over in examples. But I learned from Vlad's example that you can easily do that..

Invalid output was discussed earlier in the thread and I've added a while loop to check if the a valid number was entered, otherwise it asks for a new one.

Yes, I am aware that it would be miles better, but that's the way you learn. You write the first thing that comes to your mind when you think how you could check it, then you simplify it. I see after loads of replies on this thread that I've done that one quite badly.

Thanks though, as said earlier, I appreciate every bit of help I can get. The earlier I can fix bad habits, the better..

One question that never got answered. Why would you not use using namespace std;? And instead write std:: in front of every cin / cout / endl ?
Wouldn't it be easier to use:

using std:cout;
using std:cin;
using std:endl;

at the top if you don't want to use the whole namespace? This is just something I read from other threads that discussed this matter. They pretty much said it was preference. But apparently you shouldn't use using namespace std; in header files, why is that?

[Lowest0ne]

I see after loads of replies on this thread that I've done that one quite badly

On the contrary, the amount of comments is a good thing. Maybe your code wasn't the "best" it could be, but a program that works is not something to scoff at.

@catfish: When your user enters a valid number the buffer does not cleared.

And of course:
std::cout << year << " is " << (isLeapYear(year) ? "" : "not ") << "a leap year.";
:p

[TheIdeasMan]

Hi Reewr

But apparently you shouldn't use using namespace std; in header files, why is that?

Because the std namespace has heaps of stuff in it. It won't make a jot of difference to your program, but it makes it a lot easier for the compiler / optimiser.

using std:cout;
using std::cin;
using std::endl;
using std::string 

Is what I do, but other people prefer to put std:: before each thing. I suppose you could get quite a few at the top of your file. However if you only have 4 and use them 100 times......

Hope this helps