My first program! :D Looking for constructive criticism.

[Chervil]

As for comments, it can be hard to get the balance right. Sometimes I see comments which merely echo what the code itself says, which is a waste of time. What is more relevant is to put why something is done.

If the original requirements are very complex (in a business environment that can be the case), some sort of comment to at least tie a block of code to a particular business requirement can be essential.

[Zacharee]

@Chervil

That may seem an isolated example, but it illustrates that having "using namespace std;" can cause unexpected consequences, without it being obvious that it has even happened.

So in other words, using namespace std; utterly defeats the point in even having namespaces more or less?

@SamuelAdams

without looking at the code, in a year, you just like everyone else, will have no idea what month and dmonth represent.

I get your point but this program is just me tinkering trying to reinforce what i've learned so far, so I don't see myself returning to it a year, or even a week from now.

I also don't see any checks to make sure that the date entered is valid.
So if the user enters 13 for the month, the program just accepts that, even though it's wrong.

I have no idea how to do that yet. I'm not asking for a how-to right now but maybe could you tell me what part of the tutorial on this site describes the necessary components to do this? I like to figure things out myself :) Or if you're familiar with Bruce Eckel's Thinking in C++ let me know which chapter(s) might be helpful, as that is the book I'm learning from.

I also don't see any checks to make sure that the date entered is valid.
So if the user enters 13 for the month, the program just accepts that, even though it's wrong.

Same as above.

Last, when I declare int x; I like to set the value, else the computer uses memory that may already have a value.

Not exactly sure what this means. Are you saying that if I define a variable without initializing it, that the address used for that variable might already have a value stored from a previously run program, and said value may inadvertantly be assigned to my variable? If that's the case I can see where that may cause problems, thanks for the advice.

Off-topic here, but a suggestion i would like to make for this site: Have a reply button on page one and/or make it so you can flip pages with the reply box open.

[Zacharee]

Regarding disallowing invalid month inputs, here was the solution I come up with from what I know so far:

    cmonth:
    cout << "\nCurrent month: ";
    cin >> dmonth;
    if(dmonth > 12)
    {
        cout << "This is not a valid month." << endl;
        goto cmonth;
    }

Yes I know I'm lacking imagination with my variables labels. Yes I also know Bruce Eckel says using goto is bad programming practice in situations where it can be avoided, though He doesn't really explain why. If any of you agree with him , could you explain why to me?

I'm a little stumped about how to do this with with the day though. I'm sure I could work it out with some convoluted series of if-else statements, but I'm getting a nagging feeling theres an easier way to do it that I havn't been introduced to yet.

[booradley60]

RE: disallowing invalid month inputs

//...
    int dmonth;
    while ((std::cout << "\nCurrent month: ") &&
          (!(std::cin >> dmonth) || dmonth < 1 || dmonth > 12))
    {
        std::cout << "This is not a valid month." << std::endl;
        std::cin.clear();
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
    }
//... 

Pretty much stole this code word-for-word from here:
http://www.parashift.com/c++-faq/istream-and-ignore.html
Doing it this way you can avoid goto and you can handle bad non-numeric input, too. So 'asfdfasj' as an input by the user won't blow up your program. However, cin is going to pull from the stream until it finds the first non-numeric, so strings like '11asdf' WILL get past this check and leave 'asdf' in the input buffer, so be careful! To mitigate this, you may want to put in a std::cin.sync(); after the closing brace for the while loop.

[LB]

Zacharee wrote:

Off-topic here, but a suggestion i would like to make for this site: Have a reply button on page one and/or make it so you can flip pages with the reply box open.

In most web browsers, you can:
-Middle click a link (e.g. page 1 or 2) to open it in a new tab
-Right click on a tab and choose Duplicate, which also copies the forward and backward history
But yeah, it would be lovely to have a button that automatically quotes a post, formatting and all.

[booradley60]

I'm a little stumped about how to do this with with the day though. I'm sure I could work it out with some convoluted series of if-else statements, but I'm getting a nagging feeling theres an easier way to do it that I havn't been introduced to yet.

I would write a function with this signature:
int DaysInMonth(int month, bool isLeapYear);
and use it similar to how you use it for month:

    int currentMonth = 2;   //I'm hard-coding for this example, but get this from user
    bool isLeapYear = false;//calculate this based on the year that the user gave you
    int daysInCurrMonth = DaysInMonth(currentMonth, isLeapYear);

    int dday;
    while ((std::cout << "\nCurrent day: ") &&
          (!(std::cin >> dday) || dday < 1 || dday > daysInCurrMonth))
    {
        std::cout << "This is not a valid day in this month." << std::endl;
        std::cin.clear();
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
    }

[Zacharee]

@booradley60

So 'asfdfasj' as an input by the user won't blow up your program.

Whooaa I just input "asdf" and you're right it does blow up my program! Why does that happen!

I've been working on a function that will test the day input and prevent invalids here it is so far:

void invalidDayTest(int monthTest, int dayTest)
{
    switch(monthTest)
    {
        case 1 : if(dayTest > 31)
            {
            cout << "This is an invalid day."; goto cday;
            } break;
        case 2 : if(dayTest > 29)
            {
                cout << "This is an invalid day."; goto cday;
            } break;
        case 3 : if(dayTest > 31)
            {
                cout << "This is an invalid day."; goto cday;
            } break;
        case 4 : if(dayTest > 30)
            {
                cout << "This is an invalid day."; goto cday;
            } break;
        case 5 : if(dayTest > 31)
            {
                cout << "This is an invalid day."; goto cday;
            } break;
        case 6 : if(dayTest > 30)
            {
                cout << "This is an invalid day."; goto cday;
            } break;
        case 7 : if(dayTest > 31)
            {
                cout << "This is an invalid day."; goto cday;
            } break;
        case 8 : if(dayTest > 31)
            {
                cout << "This is an invalid day."; goto cday;
            } break;
        case 9 : if(dayTest > 30)
            {
                cout << "This is an invalid day."; goto cday;
            } break;
        case 10 : if(dayTest > 31)
            {
                cout << "This is an invalid day."; goto cday;
            } break;
        case 11 : if(dayTest > 30)
            {
                cout << "This is an invalid day."; goto cday;
            } break;
        case 12 : if(dayTest > 31)
            {
                cout << "This is an invalid day."; goto cday;
            } break;
    }
}

and then I (try to) use this in main like so:

    cday:
    cout << "Current day: ";
    cin >> dday;
    invalidDayTest(dmonth, dday);

not sure if void was the correct identifier to use, it seemed logical since i'm no using any return value from this function. The idea is that when the if statements in any given case test true the program returns to the "Current day: " I/O dialogue, and if none test true it continues with the program. However I've concluded goto cday; in my invalidDayTest function is not sufficient to define the label cday in main (The error i get is "error: label 'cday' is used but not defined.) Is the fix just to define cday at some place in main? If so my book hasn't yet taught me how do define labels other than using goto.

[Zacharee]

In most web browsers, you can:
-Middle click a link (e.g. page 1 or 2) to open it in a new tab
-Right click on a tab and choose Duplicate, which also copies the forward and backward history
But yeah, it would be lovely to have a button that automatically quotes a post, formatting and all.

The whole reason I suggested this is to avoid needing to use tabs and do it all on the current page. Also quote button would be legit, this forum seems a little outdated in that regard, almost every forum I've ever been on has had a quote button...

[booradley60]

See my post about validating the days without using goto. I suggested using a function because you could use it to separate your day validation logic from your interaction with the user. You can see how this function could be useful:

//Takes a number corresponding to a month, and a boolean indicator
//of whether or not to make leap year considerations for the month
//of February. If the month passed in is valid, it will return the
//number of days in that month, otherwise it will return 0.
unsigned int DaysInMonth(int month, bool isLeapYear)
{
    const int ERROR_VAL = 0;

    switch(month)
    {
        //cases without a 'break' or 'return' fall through automatically to the next case
        case 1 :
        case 3 :
        case 5:
        case 7:
        case 8:
        case 10:
        case 12:
            return 31;
        case 4:
        case 6:
        case 9:
        case 11:
            return 30;
        case 2 : 
            //check out http://cplusplus.com/doc/tutorial/operators/ and look for '?' operator
            return (isLeapYear ? 29 : 28); 
        default:
            return ERROR_VAL;
        
    }
}

[Zacharee]

@booradley
wow i totally missed your first post about validating the days. I'm a little confused about how you're using bool isLeapYear. To my untrained eye it appears that you are initializing it as false and leaving no mechanism to change it to true. Is this bool based on the year input? If so would I have to shift somethings around so that the use inputs the year first so the program can test for a leap year before the day input? While I value you're contribution, it is getting a little out of my confort zone and I'm hestitant to use anything I don't understand in fear of building a habit of doing such. Although I probably will try to incorporate this eventually, because in theory your solution doesn't include anything I havn't learned yet, its just the way you are using some things that is intimidating me. So for now all I'm asking for is an explanation as to why my solution didn't work, and if possible, how to make it work. Remember I've only been at this for 3 days... baby steps XD

edit: I do like how you used the switch cases falling through, I had forgotten switches could behave like that. I think I'll make a change to my invalidDayTest function to try and take advantage of this.

[Zacharee]

Does the answer lie somewhere in pointers or references? I remember reading that pointers can be used to modify outside objects, but I'm not sure how this would work with a label. I also remember reading that you cant have a goto [label] before the label appears in the code. Can pointers be used with labels? If not where would a put a function prototype that uses a goto who's label appears in main? Is it time for me to start a new topic since my questions are becoming less to do with my original idea of "constructive criticism"?

[Lowest0ne]

@booradley60: you try to return -1 in a function that returns an unsigned int. I would say that zero makes a better error value;

@Zacharee: Never use goto, simple and plain.

To my untrained eye it appears that you are initializing it as false and leaving no mechanism to change it to true

I don't see booradley making it true either. I imagine he/she leaves it up to you :)

// I changed the names
leapYear = isLeapYear(year); // write that function 

My teacher gave me the magic number 4. If you have to write the same thing four times, it should be stored as a variable/made into a function.

I don't think it is ever too early to at least see more advanced techniques. Sometimes they are even simple:

#include <iostream>

const char* month_as_string(unsigned int month_as_int){
  static const char* months[] = {
    "January",  "February", "March",   "April",
    "May",      "June",     "July",    "August",
    "September","October",  "November","December",
    "Invalid"
  };  
  return month_as_int < 12 ? months[month_as_int] : months[12];
}
bool valid_month(unsigned int month){
  return month < 12;
}

int days_in_month(unsigned int month_as_int){
  static unsigned int days[]={31,30, 28, 30, 31, 30, 31, 31, 30, 31, 30, 31};
  return month_as_int < 12 ? days[month_as_int] : -1;
}
int main(void)
{
  for (int i = -3; i < 15; i++)
    if (valid_month(i))
      std::cout << month_as_string(i) << " " << days_in_month(i) << '\n';
      
  std::cout << "Bad Input: " << month_as_string(12) << " "
            << days_in_month(12) << '\n';
  return 0;
}

// You can do something like
if (days_in_month(2) < this_day) std::cout << "Invalid day\n";

We've basically removed the switches and put the wanted values in an array. If the given input is valid, the wanted value is returned. Otherwise you get an error value.

The keyword static means that memory is allocated for the data once, and other calls to the function use the same block of memory.

Twelve is a magic number, and it is a problem. The ideal solution here is to make a class, but I think too far ahead for you.

[booradley60]

@LowestOne

@booradley60: you try to return -1 in a function that returns an unsigned int. I would say that zero makes a better error value;

Good catch. I'll edit my example. Usually I'm good about compiling these before I post. Sorry if I've created any confusion!

EDIT: I like your array solution, but I think the valid month range should be 1-12 instead of 0-11. This range more closely fits the real-world domain in which dates are typically 1-based instead of 0-based. "Invalid" could be moved to months[0] and your valid_month predicate would have to change slightly. Also, I'm looking at it trying to think of the cleanest way to account for leap years...

[Lowest0ne]

I actually spent some time making a Date class last night. The zero based ranged is a little confusing, I think you are right. On the other hand, this is ideally inside a class, and the class would be able to bridge that gap between what a person expects and the way the class works. When you start to have some classes with zero index and others with 1 index things can get confusing.

Thinking about leap year, I consider this:

unsigned int days_in_month(const unsigned int month, const bool leap){
  static const unsigned int month_count = 12
  static const unsigned int days[][month_count]= {
    {31,30, 28, 30, 31, 30, 31, 31, 30, 31, 30, 31},
    {31,30, 29, 30, 31, 30, 31, 31, 30, 31, 30, 31}
  };
  return month - 1 < month_count ? days[leap ? 1 : 0][month - 1] : 0;
}

Edit, and if you were really concerned about space, make days a char array.