Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link
  • Do not use using namespace std in global scope Do not use using namespace std in global scope.

  • Never use global variables. Right away, this will introduce all sorts of problems, including maintainability and bugs. There are different alternatives to this, one being a struct:

    Such a structure just needs the month, day, and year:

     struct Date
     {
     int month;
     int day;
     int year;
     };
    

    Initialize the two Date instances:

     // same order as appears in struct
     Date date1 = {1, 2, 2000};
     Date date2 = {4, 5, 2001};
    

    Access the structures and set the data members (with your code):

     std::cout << "Enter first date: ";
     std::cin >> date1.year >> date1.month >> date1.day;
     std::cout << "Enter second date: ";
     std::cin >> date2.year >> date2.month >> date2.day;
    

    If you want to get into encapsulation/information-hiding, I'd recommend a class instead. If you want to keep this simpler than a struct, just move the globals into main() (but use the variables from my example). You could also create more specialized functions, thereby not just working in main(). Modularity will help keep your code more organized and maintainable.

  • I don't like int for these values (dates cannot be negative). I'd go with std::size_t instead (include <cstddef> to use it).

  • month_days[] should be a const while in global scope. As a constant, it can remain there because it cannot be changed by anything else. However, this will prevent you from accounting for leap-years. Speaking of which...

  • To account for leap-years, I'd either:

  1. leave out February's value from the array (it's the only value that could change)
  2. not make the array a constant (the program will handle the values during runtime)

With that, you can allow the program to adjust February's value if a leap-year.

  • Do not use using namespace std in global scope.

  • Never use global variables. Right away, this will introduce all sorts of problems, including maintainability and bugs. There are different alternatives to this, one being a struct:

    Such a structure just needs the month, day, and year:

     struct Date
     {
     int month;
     int day;
     int year;
     };
    

    Initialize the two Date instances:

     // same order as appears in struct
     Date date1 = {1, 2, 2000};
     Date date2 = {4, 5, 2001};
    

    Access the structures and set the data members (with your code):

     std::cout << "Enter first date: ";
     std::cin >> date1.year >> date1.month >> date1.day;
     std::cout << "Enter second date: ";
     std::cin >> date2.year >> date2.month >> date2.day;
    

    If you want to get into encapsulation/information-hiding, I'd recommend a class instead. If you want to keep this simpler than a struct, just move the globals into main() (but use the variables from my example). You could also create more specialized functions, thereby not just working in main(). Modularity will help keep your code more organized and maintainable.

  • I don't like int for these values (dates cannot be negative). I'd go with std::size_t instead (include <cstddef> to use it).

  • month_days[] should be a const while in global scope. As a constant, it can remain there because it cannot be changed by anything else. However, this will prevent you from accounting for leap-years. Speaking of which...

  • To account for leap-years, I'd either:

  1. leave out February's value from the array (it's the only value that could change)
  2. not make the array a constant (the program will handle the values during runtime)

With that, you can allow the program to adjust February's value if a leap-year.

  • Do not use using namespace std in global scope.

  • Never use global variables. Right away, this will introduce all sorts of problems, including maintainability and bugs. There are different alternatives to this, one being a struct:

    Such a structure just needs the month, day, and year:

     struct Date
     {
     int month;
     int day;
     int year;
     };
    

    Initialize the two Date instances:

     // same order as appears in struct
     Date date1 = {1, 2, 2000};
     Date date2 = {4, 5, 2001};
    

    Access the structures and set the data members (with your code):

     std::cout << "Enter first date: ";
     std::cin >> date1.year >> date1.month >> date1.day;
     std::cout << "Enter second date: ";
     std::cin >> date2.year >> date2.month >> date2.day;
    

    If you want to get into encapsulation/information-hiding, I'd recommend a class instead. If you want to keep this simpler than a struct, just move the globals into main() (but use the variables from my example). You could also create more specialized functions, thereby not just working in main(). Modularity will help keep your code more organized and maintainable.

  • I don't like int for these values (dates cannot be negative). I'd go with std::size_t instead (include <cstddef> to use it).

  • month_days[] should be a const while in global scope. As a constant, it can remain there because it cannot be changed by anything else. However, this will prevent you from accounting for leap-years. Speaking of which...

  • To account for leap-years, I'd either:

  1. leave out February's value from the array (it's the only value that could change)
  2. not make the array a constant (the program will handle the values during runtime)

With that, you can allow the program to adjust February's value if a leap-year.

added 156 characters in body
Source Link
Jamal
  • 35.2k
  • 13
  • 134
  • 238
  • Do not use using namespace std in global scope.

  • Never use global variables. Right away, this will introduce all sorts of problems, including maintainability and bugs. There are different alternatives to this, one being a struct:

    Such a structure just needs the month, day, and year:

     struct Date
     {
     int month;
     int day;
     int year;
     };
    

    Initialize the two Date instances:

     // same order as appears in struct
     Date date1 = {1, 2, 2000};
     Date date2 = {4, 5, 2001};
    

    Access the structures and set the data members (with your code):

     std::cout << "Enter first date: ";
     std::cin >> date1.year >> date1.month >> date1.day;
     std::cout << "Enter second date: ";
     std::cin >> date2.year >> date2.month >> date2.day;
    

    If you want to get into encapsulation/information-hiding, I'd recommend a class instead. If you want to keep this simpler than a struct, just move the globals into main() (but use the variables from my example). You could also create more specialized functions, thereby not just working in main(). Modularity will help keep your code more organized and maintainable.

  • month_days[] should be a const. As a constant, it can remain in global scope because it cannot be changed by anything else.

  • I don't like int for datesthese values (theydates cannot be negative). I'd go with std::size_t instead (include <cstddef> to use it).

  • month_days[] should be a const while in global scope. As a constant, it can remain there because it cannot be changed by anything else. However, this will prevent you from accounting for leap-years. Speaking of which...

  • To account for leap-years, I'd either:

  1. leave out February's value from the array (it's the only value that could change)
  2. not make the array a constant (the program will handle the values during runtime)

With that, you can allow the program to adjust February's value if a leap-year.

  • Do not use using namespace std in global scope.

  • Never use global variables. Right away, this will introduce all sorts of problems, including maintainability and bugs. There are different alternatives to this, one being a struct:

    Such a structure just needs the month, day, and year:

     struct Date
     {
     int month;
     int day;
     int year;
     };
    

    Initialize the two Date instances:

     // same order as appears in struct
     Date date1 = {1, 2, 2000};
     Date date2 = {4, 5, 2001};
    

    Access the structures and set the data members (with your code):

     std::cout << "Enter first date: ";
     std::cin >> date1.year >> date1.month >> date1.day;
     std::cout << "Enter second date: ";
     std::cin >> date2.year >> date2.month >> date2.day;
    

    If you want to get into encapsulation/information-hiding, I'd recommend a class instead. If you want to keep this simpler than a struct, just move the globals into main() (but use the variables from my example). You could also create more specialized functions, thereby not just working in main(). Modularity will help keep your code more organized and maintainable.

  • month_days[] should be a const. As a constant, it can remain in global scope because it cannot be changed by anything else.

  • I don't like int for dates (they cannot be negative). I'd go with std::size_t instead (include <cstddef> to use it).

  • Do not use using namespace std in global scope.

  • Never use global variables. Right away, this will introduce all sorts of problems, including maintainability and bugs. There are different alternatives to this, one being a struct:

    Such a structure just needs the month, day, and year:

     struct Date
     {
     int month;
     int day;
     int year;
     };
    

    Initialize the two Date instances:

     // same order as appears in struct
     Date date1 = {1, 2, 2000};
     Date date2 = {4, 5, 2001};
    

    Access the structures and set the data members (with your code):

     std::cout << "Enter first date: ";
     std::cin >> date1.year >> date1.month >> date1.day;
     std::cout << "Enter second date: ";
     std::cin >> date2.year >> date2.month >> date2.day;
    

    If you want to get into encapsulation/information-hiding, I'd recommend a class instead. If you want to keep this simpler than a struct, just move the globals into main() (but use the variables from my example). You could also create more specialized functions, thereby not just working in main(). Modularity will help keep your code more organized and maintainable.

  • I don't like int for these values (dates cannot be negative). I'd go with std::size_t instead (include <cstddef> to use it).

  • month_days[] should be a const while in global scope. As a constant, it can remain there because it cannot be changed by anything else. However, this will prevent you from accounting for leap-years. Speaking of which...

  • To account for leap-years, I'd either:

  1. leave out February's value from the array (it's the only value that could change)
  2. not make the array a constant (the program will handle the values during runtime)

With that, you can allow the program to adjust February's value if a leap-year.

added 156 characters in body
Source Link
Jamal
  • 35.2k
  • 13
  • 134
  • 238
  • Do not use using namespace std in global scope.

  • Never use global variables. Right away, this will introduce all sorts of problems, including maintainability and bugs. There are different alternatives to this, one being a struct:

    Such a structure just needs the month, day, and year:

     struct Date
     {
     int first_date_month;
     int first_date_days;
     int first_date_year;
     int second_date_month;
     int second_date_days;month;
     int second_date_year;day;
     int days;year;
     };
    

    Initialize athe two Date instanceinstances:

     // same order as appears in struct
     Date datedate1 = {1, 2, 3, 4, 5, 6, 302000}; // sameDate orderdate2 as= appears{4, in5, struct2001};
    

    Access the structstructures and set athe data membermembers (with your code):

     std::cout << "Enter first date: ";
     std::cin >> datedate1.first_date_yearyear >> datedate1.first_date_monthmonth >> date1.day;
     std::cout << "Enter second date: ";
     std::cin >> date2.first_date_days;year >> date2.month >> date2.day;
    

    If you want to get into encapsulation/information-hiding, I'd recommend a class instead. If you want to keep this simple, more sosimpler than a struct, then just move the globals into main() (but use the variables from my example). You could also create more specialized functions, thereby not just working in main(). Modularity will help keep your code more organized and maintainable.

  • month_days[] should be a const. Also, as As a constant, it can remain in global scope because it cannot be changed by anything else.

  • I don't like int for dates (they cannot be negative). I'd go with std::size_t instead (include <cstddef> to use it).

  • Do not use using namespace std in global scope.

  • Never use global variables. Right away, this will introduce all sorts of problems, including maintainability and bugs. There are different alternatives to this, one being a struct:

     struct Date
     {
     int first_date_month;
     int first_date_days;
     int first_date_year;
     int second_date_month;
     int second_date_days;
     int second_date_year;
     int days;
     };
    

    Initialize a Date instance:

     Date date = {1, 2, 3, 4, 5, 6, 30}; // same order as appears in struct
    

    Access the struct and set a data member (with your code):

     std::cout << "Enter first date: ";
     std::cin >> date.first_date_year >> date.first_date_month >> date.first_date_days;
    

    If you want to get into encapsulation/information-hiding, I'd recommend a class instead. If you want to keep this simple, more so than a struct, then just move the globals into main(). You could also create more specialized functions, thereby not just working in main().

  • month_days[] should be a const. Also, as a constant, it can remain in global scope.

  • I don't like int for dates (they cannot be negative). I'd go with std::size_t instead (include <cstddef> to use it).

  • Do not use using namespace std in global scope.

  • Never use global variables. Right away, this will introduce all sorts of problems, including maintainability and bugs. There are different alternatives to this, one being a struct:

    Such a structure just needs the month, day, and year:

     struct Date
     {
     int month;
     int day;
     int year;
     };
    

    Initialize the two Date instances:

     // same order as appears in struct
     Date date1 = {1, 2, 2000}; Date date2 = {4, 5, 2001};
    

    Access the structures and set the data members (with your code):

     std::cout << "Enter first date: ";
     std::cin >> date1.year >> date1.month >> date1.day;
     std::cout << "Enter second date: ";
     std::cin >> date2.year >> date2.month >> date2.day;
    

    If you want to get into encapsulation/information-hiding, I'd recommend a class instead. If you want to keep this simpler than a struct, just move the globals into main() (but use the variables from my example). You could also create more specialized functions, thereby not just working in main(). Modularity will help keep your code more organized and maintainable.

  • month_days[] should be a const. As a constant, it can remain in global scope because it cannot be changed by anything else.

  • I don't like int for dates (they cannot be negative). I'd go with std::size_t instead (include <cstddef> to use it).

added 265 characters in body
Source Link
Jamal
  • 35.2k
  • 13
  • 134
  • 238
Loading
Source Link
Jamal
  • 35.2k
  • 13
  • 134
  • 238
Loading
lang-cpp

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