Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

Don't do this, compare for example Why is "using namespace std" in C++ considered bad practice? Why is "using namespace std" in C++ considered bad practice?. And actually your code does not rely on it, you can simply remove that line.

As already mentioned in the comments, neither the argc/argv parameters nor the return statement is required in C++ (see e.g. What should main() return in C and C++? What should main() return in C and C++? for an overview):

Don't do this, compare for example Why is "using namespace std" in C++ considered bad practice?. And actually your code does not rely on it, you can simply remove that line.

As already mentioned in the comments, neither the argc/argv parameters nor the return statement is required in C++ (see e.g. What should main() return in C and C++? for an overview):

Don't do this, compare for example Why is "using namespace std" in C++ considered bad practice?. And actually your code does not rely on it, you can simply remove that line.

As already mentioned in the comments, neither the argc/argv parameters nor the return statement is required in C++ (see e.g. What should main() return in C and C++? for an overview):

added 873 characters in body
Source Link
Martin R
  • 24.2k
  • 2
  • 38
  • 96
void askForBirthdate(int &dayOfBirth, int &monthOfBirth, int &yearOfBirth){
 // ...
}
int main()
{
 int dayOfBirth, monthOfBirth, yearOfBirth;
 askForBirthdate(dayOfBirth, monthOfBirth, yearOfBirth);
 int ageInSeconds = calcAgeInSeconds(dayOfBirth, monthOfBirth, yearOfBirth);
 std::cout << "You are " << ageInSeconds << "old.\n";
}

Converting day/month/year to a time value (according to the local timezone) is easily done with mktime() (the "counterpart" to localtime()):

time_t timeForDate(int day, int month, int year){
 struct tm timeinfo = { 0 };
 timeinfo.tm_mday = day;
 timeinfo.tm_mon = month - 1;
 timeinfo.tm_year = year - 1900;
 timeinfo.tm_isdst = -1;
 return mktime(&timeinfo);
}

Some other miscelleaneous remarks:

  • You can get rid of the function declarations if you define all functions before using them.

  • Statements like

     int ageInSeconds=0;
     ageInSeconds=calcAgeInSeconds(monthBorn,dayOfMonthBorn,yearBorn);
    

can be combined to

 int ageInSeconds = calcAgeInSeconds(monthBorn, dayOfMonthBorn, yearBorn);

And use more (horizontal) space!

  • Check and fix all compiler warnings. For example, in calcOffset(), curYear and three more variables are computed but their values are never used.

  • Always use curly braces { } with if-statements, even if the if or else part consists only of a single statement. That helps to avoid errors if the code is edited later.

  • The long if/else if/else if/... statement in calcDaysRemainingThisYear() can be simplified by using a switch statement.

void askForBirthdate(int &dayOfBirth, int &monthOfBirth, int &yearOfBirth){
 // ...
}
int main()
{
 int dayOfBirth, monthOfBirth, yearOfBirth;
 askForBirthdate(dayOfBirth, monthOfBirth, yearOfBirth);
 int ageInSeconds = calcAgeInSeconds(dayOfBirth, monthOfBirth, yearOfBirth);
 std::cout << "You are " << ageInSeconds << "old.\n";
}

Converting day/month/year to a time value (according to the local timezone) is easily done with mktime():

time_t timeForDate(int day, int month, int year){
 struct tm timeinfo = { 0 };
 timeinfo.tm_mday = day;
 timeinfo.tm_mon = month - 1;
 timeinfo.tm_year = year - 1900;
 timeinfo.tm_isdst = -1;
 return mktime(&timeinfo);
}
void askForBirthdate(int &dayOfBirth, int &monthOfBirth, int &yearOfBirth){
 // ...
}
int main()
{
 int dayOfBirth, monthOfBirth, yearOfBirth;
 askForBirthdate(dayOfBirth, monthOfBirth, yearOfBirth);
 int ageInSeconds = calcAgeInSeconds(dayOfBirth, monthOfBirth, yearOfBirth);
 std::cout << "You are " << ageInSeconds << "old.\n";
}

Converting day/month/year to a time value (according to the local timezone) is easily done with mktime() (the "counterpart" to localtime()):

time_t timeForDate(int day, int month, int year){
 struct tm timeinfo = { 0 };
 timeinfo.tm_mday = day;
 timeinfo.tm_mon = month - 1;
 timeinfo.tm_year = year - 1900;
 timeinfo.tm_isdst = -1;
 return mktime(&timeinfo);
}

Some other miscelleaneous remarks:

  • You can get rid of the function declarations if you define all functions before using them.

  • Statements like

     int ageInSeconds=0;
     ageInSeconds=calcAgeInSeconds(monthBorn,dayOfMonthBorn,yearBorn);
    

can be combined to

 int ageInSeconds = calcAgeInSeconds(monthBorn, dayOfMonthBorn, yearBorn);

And use more (horizontal) space!

  • Check and fix all compiler warnings. For example, in calcOffset(), curYear and three more variables are computed but their values are never used.

  • Always use curly braces { } with if-statements, even if the if or else part consists only of a single statement. That helps to avoid errors if the code is edited later.

  • The long if/else if/else if/... statement in calcDaysRemainingThisYear() can be simplified by using a switch statement.

Source Link
Martin R
  • 24.2k
  • 2
  • 38
  • 96
using namespace std;

Don't do this, compare for example Why is "using namespace std" in C++ considered bad practice?. And actually your code does not rely on it, you can simply remove that line.


As already mentioned in the comments, neither the argc/argv parameters nor the return statement is required in C++ (see e.g. What should main() return in C and C++? for an overview):

int main() 
{
 welcome();
}

Your welcome() function does all the work (which makes the function name quite misleading). A better design would be to separate between input, calculation, and output:

void askForBirthdate(int &dayOfBirth, int &monthOfBirth, int &yearOfBirth) {
 // ...
}
int main()
{
 int dayOfBirth, monthOfBirth, yearOfBirth;
 askForBirthdate(dayOfBirth, monthOfBirth, yearOfBirth);
 int ageInSeconds = calcAgeInSeconds(dayOfBirth, monthOfBirth, yearOfBirth);
 std::cout << "You are " << ageInSeconds << "old.\n";
}

(This will be revised below.)


Your age calculation is quite complicated and has errors. As pointed out in the other answers:

  • The leap year algorithm is not correct.
  • Daylight save time transitions are not considered.
  • The current time is retrieved multiple times which can cause inconsistent results.

Instead of converting the current time to day/month/year/... and computing the difference to the birth day/month/year, it would be easier to go the other way around: Convert the birth date to a time value (seconds since Jan 1, 1970 UTC) and compute the difference to the current time value (which is obtained by time(0).

Converting day/month/year to a time value (according to the local timezone) is easily done with mktime():

time_t timeForDate(int day, int month, int year) {
 struct tm timeinfo = { 0 };
 timeinfo.tm_mday = day;
 timeinfo.tm_mon = month - 1;
 timeinfo.tm_year = year - 1900;
 timeinfo.tm_isdst = -1;
 return mktime(&timeinfo);
}

The main program then becomes

int main()
{
 int dayOfBirth, monthOfBirth, yearOfBirth;
 askForBirthdate(dayOfBirth, monthOfBirth, yearOfBirth);
 time_t birthTime = timeForDate(dayOfBirth, monthOfBirth, yearOfBirth);
 time_t nowTime = time(0);
 time_t ageInSeconds = nowTime - birthTime;
 std::cout << "You are " << ageInSeconds << " seconds old.\n";
}
lang-cpp

AltStyle によって変換されたページ (->オリジナル) /