8
\$\begingroup\$

I'm new to coding C++ and I want to get some insight on my progress. Can you guys take a look at my program and give me some feedback on it?

#include <iostream>
using namespace std;

int main(){

int day{};
int monthNum{};
cout << "Calculate how many days are left in the month. \nPlease enter the month number and the day, seperated by a space. ex 1 22 for Jan 22nd: ";
cin >> monthNum >> day;

if (monthNum < 1 || monthNum > 12 || day < 1 || day > 31 || (monthNum == 2 && (day < 1 || day > 29))){
    cout << "Error, not a valid input. \nPlease make sure you're month and day are valid." << endl;
    return 0;
}

int daysInCurrentMonth{};
char leapYear;
int doNothing{};  //Used to do nothing if certain conditions are false

if (monthNum == 2){
    cout << "\nIs it leap year? Type y for yes and n for no. ";
    cin >> leapYear;
    ((leapYear == 'y') ? daysInCurrentMonth = 29 : daysInCurrentMonth = 28);
    if (leapYear != 'y' && leapYear != 'n'){
        cout << "invalid input..." << endl << endl;
        return 0;
    }else{ 
        doNothing = 0;
    }
}

cout << "\nYou entered day " << day << " of month " << monthNum << endl;

((monthNum == 4 || monthNum == 6 || monthNum == 9 || monthNum == 11) ? daysInCurrentMonth = 30 : doNothing = 0);
((monthNum == 1 || monthNum == 3 || monthNum == 5 || monthNum == 7 || monthNum == 8 || monthNum == 10 || monthNum == 12) ? daysInCurrentMonth = 31 : doNothing = 0);

int daysLeftInMonth{daysInCurrentMonth - day};
cout << "\nThere are " << daysLeftInMonth << " days left in the month." << endl;

cout << endl;
return 0;
}

This is my third iteration of this program. This is the tutorial series that I've been watching. I'm currently on section 9 and just finished learning about conditional operators. I'm trying to refine the project as I go along. Also, I know it's pretty early to be thinking about this, but if anyone is able to, I'd like to know if the way I structure this is in the right direction for becoming a professional programmer, i.e. it's easy to follow and is readable, or it looks like garbage.

\$\endgroup\$
  • 1
    \$\begingroup\$ Line 12: you're should be your :P I'm sorry, I know it's a little thing but I can't stop thinking about it. \$\endgroup\$ – DarkMatterMatt Jun 24 '18 at 9:16
5
\$\begingroup\$
  • Don't use using namespace std

  • Prefer using \n over std::endl

  • The last return 0 is not necessary in this case as the compiler will generate it for you

  • When using the ternary operator you can just assign at the start instead of having to do it in both branches

  • Not part of the code but you should check your spelling

I'm not sure if you do the input on purpose to learn how it works but it can be done without user interaction.
Have a look at ctime as it offers many functions that can be useful here.

Here's a small program showcasing how to use those functions:

#include <ctime>
#include <iostream>

int main() {
    std::time_t time = std::time(nullptr);
    auto today = std::localtime(&time);

    int day = today->tm_mday;
    int month = today->tm_mon + 1;
    int year = today->tm_year + 1900;
    int month_length = 30;

    constexpr int february = 2;
    if (month == february) {
        month_length = is_leapyear(year) ? 29 : 28;
    }

    std::cout << month_length - day << " days remain in this month.\n";
}

Since the is_leapyear function is trivial I left it out.

Of course if you're using C++20 then you can just use chrono which offers an amazing amount of time and date related functions.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Wow that is a million times easier. I did the inputs because as of where I am in my course, its as far as I've gotten. Thanks a bunch for the insight. Definitely switching to std:: and \n where I can \$\endgroup\$ – Thomas Margraff Jun 23 '18 at 16:01
  • \$\begingroup\$ @Yuri This was just to showcase <ctime> and not to reimplement the program, right? Or did we forget most months have 31 days, not 30... \$\endgroup\$ – corsiKa Jun 24 '18 at 12:50
5
\$\begingroup\$

As Yuri already said in his answer, don't use using namespace std. I also recommend that you read on this site some of the Q&A in the tag , you'll learn a bunch of this type of common beginner mistakes.

Your variable names are great, it's clear what their purpose is without having to add any comments.

doNothing has no purpose. You set it in various places, but you don't read it anywhere. However, if you did use it, I would recommend that you declare it as a bool (which can have values true or false). Finally, you initialize it as int doNothing{};, which gives it a value of 0, and everywhere where you assign values to it, you're assigning 0 too, thus these assignments don't do anything.

This bit of code:

if (monthNum == 2){
    //...
}
((monthNum == 4 || monthNum == 6 || monthNum == 9 || monthNum == 11) ? daysInCurrentMonth = 30 : doNothing = 0);
((monthNum == 1 || monthNum == 3 || monthNum == 5 || monthNum == 7 || monthNum == 8 || monthNum == 10 || monthNum == 12) ? daysInCurrentMonth = 31 : doNothing = 0);

has a lot of redundancy. You test for month 2, then for the 30-day months, then for the remaining 7 months. You could simplify this as:

if (monthNum == 2) {
    // 28/29 days
} else if (monthNum == 4 || monthNum == 6 || monthNum == 9 || monthNum == 11) {
    // 30 days
} else {
    // 31 days
}

But it would be even neater to set up a look-up table:

int daysInMonth[] = {0,31,28,31,30,31,30,31,31,30,31,30,31}; // element[0] not used
int daysInCurrentMonth = daysInMonth[monthNum];
// ...and then check for leap years

Much of the logic in main could be separated out into functions. The shorter each function is, the easier it is to verify it is correct, and the easier it is to read it.

Declare variables within the scope where you use them:

char leapYear;
if (monthNum == 2){
    cout << "\nIs it leap year? Type y for yes and n for no. ";
    cin >> leapYear;
    //...
}
//... leapYear is never used from here on

You should instead declare leapYear inside the scope of the if statement, so it will not exist outside this scope. The shorter the scope of variables is, the easier it is to read the program and the fewer bugs you'll make.

When exiting your program with an error, it is common to return a non-zero value (at least on POSIX systems, I don't know if Windows does anything with the return value?)

std::cout << "invalid input...\n\n";
return 1;

Finally, I personally find this confusing:

int day{};

It initializes day to 0, but does so in a non-obvious way. This is maybe more my personal preference than anything else, but I'd suggest

int day = 0;

if you need to initialize day, or just

int day;

since you'll be writing into it right after, and never read the value it was initialized to. Your course is probably hammering on uniform initialization. I guess I'm old-fashioned in thinking that the = operator is nice and explicit. :)

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks a bunch for the indepth feedback. It really helps. I used doNothing as a way to exit a conditional operator in the same line, without doing anything. And yes, almost every time the instructor declares a variable, he initalizes it, and says its good practice to avoid errors and warnings, also he says the {} are modern c++, so i chose that. What is a look-up table? An array? Again thanks for your input, it really helps \$\endgroup\$ – Thomas Margraff Jun 23 '18 at 17:39
  • \$\begingroup\$ Yes, it is good practice to declare variables and initialize them right away. To me that translates into “don’t declare a variable until you have a value to assign to it”, which is not always possible, but one tries... a lookup table is literally a table where you can look up stuff... often it’s an array, where you associate an integer to some value, as in this case the month number to the month length. It’s a very useful concept I think, in this case it avoids a complicated conditional statement. \$\endgroup\$ – Cris Luengo Jun 23 '18 at 18:24
  • \$\begingroup\$ By complicated conditional statements, you mean this lol. Thanks again for all of your help \$\endgroup\$ – Thomas Margraff Jun 23 '18 at 18:54
2
\$\begingroup\$

Here's a couple of style concerns to add onto what the others have said.

  1. It's standard to put spaces after parenthesis in if-statements and before and after "else". Example:

    if ([condition1]) {
        [stuff1]
    } else if ([condition2]) {
        [stuff2]
    } else {
        [stuff3]
    }
    
  2. The ternary operator is not usually used to decide between statements to execute, but rather a single value; i.e. this would be more idiomatic

    daysInCurrentMonth = leapYear == 'y' ? 29 : 28;
    

    as opposed to

    ((leapYear == 'y') ? daysInCurrentMonth = 29 : daysInCurrentMonth = 28);
    
  3. You should indent all code in the main function.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I think your point one would be very hotly debated by professional programmers. I'm also of the opinion that it should be there, but there are plenty of others who feel the space is not necessary. What is more important is that it is either consistently there or consistently not there. \$\endgroup\$ – corsiKa Jun 24 '18 at 13:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.