It's almost Friday, right?

Question

This is my final post for my CS1 saga. Mostly all I had to do was just "class-ify" my past project in this post. Here are the requirements:

Make all data members private and use constructors, accessors and mutators. Enhancement: Build data verification into the member functions.

I'm mostly concerned with me doing OO programming correctly. I wasn't very good with it in my Java days way back yonder, and I'm not sure it's that good now either. Focus in this area would be appreciated.

classyDayOfWeek.cpp:

/**
 * @file classyDayOfWeek.cpp
 * @brief Computes the day of the week given a certain date
 * @author syb0rg
 * @date 12/10/14
 */

#include <cctype>
#include <iostream>
#include <limits>

const std::string weekDays[] = {"Sunday", "Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday"};

/**
 * The class associated with date values and calculating the day of the week
 */
class Date
{
public:
    int dayOfWeek();
    void getInput();
    bool isLeapYear();
    int getCenturyValue();
    int getYearValue();
    int getMonthValue();
private:
    unsigned day = 0;
    unsigned month = 0;
    unsigned year = 0;
};

/**
 * Makes sure data isn't malicious, and signals user to re-enter proper data if invalid
 */
unsigned getSanitizedNum()
{
    unsigned input = 0;
    while(!(std::cin >> input))
    {
        // clear the error flag that was set so that future I/O operations will work correctly
        std::cin.clear();
        // skips to the next newline
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
        std::cout << "Invalid input.  Please enter a positive number: ";
    }
    return input;
}

/**
 * Safetly grabs and returns a lowercase version of the character (if the lowercase exists)
 */
char getSanitizedChar()
{
    // absorb newline character (if existant) from previous input
    if('\n' == std::cin.peek()) std::cin.ignore();
    return std::tolower(std::cin.get());
}

/**
 * Determines if the year stored in the Date instance is a leap year
 */
bool Date::isLeapYear()
{
    return (year >= 1582) && (!(year % 400) || (!(year % 4) && (year % 100)));
}

/** 
 * Finds the century, divides by 4, and returns the remainder.
 */
int Date::getCenturyValue()
{
    return (2 * (3 - div(year / 100, 4).rem));
}

/**
 * Computes a value based on the years since the beginning of the century.
 */
int Date::getYearValue()
{
    int mod = year % 100;
    return (mod + div(mod, 4).quot);
}

/**
 * Returns a value based on the given hash tables (requires invoking the isLeapYear function)
 */
int Date::getMonthValue()
{
    const static int NON_LEAP_VALS[] = { 0xDEAD, 0, 3, 3, 6, 1, 4, 6, 2, 5, 0, 3, 5 };
    const static int LEAP_VALS[] = { 0xDEAD, 6, 2, 3, 6, 1, 4, 6, 2, 5, 0, 3, 5 };
    return (isLeapYear()) ? LEAP_VALS[month] : NON_LEAP_VALS[month];
}

/**
 * Compute the sum of the date's day plus the values returned by getMonthValue, getYearValue, and getCenturyValue.
 */
int Date::dayOfWeek()
{
    return div(day + getMonthValue() + getYearValue() + getCenturyValue(), 7).rem;
}

/**
 * Get's input safely and stores the input in the instances members
 */
void Date::getInput()
{
    std::cout << "Enter the month (1-12): ";
    month = getSanitizedNum();

    std::cout << "Enter the day (1-31): ";
    day = getSanitizedNum();

    std::cout << "Enter the year: ";
    year = getSanitizedNum();
}

int main()
{
    Date date;
    do
    {
        date.getInput();
        std::cout << "The day of the week is " << weekDays[date.dayOfWeek()] << std::endl;

        std::cout << "Run the program again (y/N): ";  // signify n as default with capital letter
    } while ('y' == getSanitizedChar());
}

Answer 1

Here are a few critiques.

/**
 * Get's input safely and stores the input in the instances members
 */
void Date::getInput()
{
  std::cout << "Enter the month (1-12): ";
  month = getSanitizedNum();

  std::cout << "Enter the day (1-31): ";
  day = getSanitizedNum();

  std::cout << "Enter the year: ";
  year = getSanitizedNum();
}

This should not be a member function of the Date class. Objects should have a single responsibility (this is the Single Responsibility Principle). Objects with only a single responsibility are easier to maintain in the long run. Here, your Date class is concerned with representing a date and gathering inputs. The second responsibility should be split out. This would be better as a stand-alone function that created and returned a new Date. This means that you are going to need a constructor too (will show the code for it below).

/**
 * Makes sure data isn't malicious, and signals user to re-enter proper data if invalid
 */
unsigned getSanitizedNum()
{
  unsigned input = 0;
  while(!(std::cin >> input))
  {
      // clear the error flag that was set so that future I/O operations will work correctly
      std::cin.clear();
      // skips to the next newline
      std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
      std::cout << "Invalid input.  Please enter a positive number: ";
  }
  return input;
}

This function is not particularly flexible, and is not necessarily offering that much safety. For example, it is used to read in a month, which should be between 1 and 12, but it will accept any positive integer. Also, it does not repeat the prompt on invalid input. Repeating the prompt is more user friendly so that a user who may have mis-typed doesn't have to look several lines up to remember what they are entering. To make this more flexible, as well as safer, we can introduce some parameters:

unsigned int getSanitizedNum(std::string prompt, unsigned int min, unsigned int max)
{
    unsigned int input = 0;

    std::cout << prompt;
    while(!(std::cin >> input) || input < min || input > max)
    {
        // clear the error flag that was set so that future I/O operations will work correctly
        std::cin.clear();
        // skips to the next newline
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
        std::cout << "Invalid input. " << prompt;
    }
    return input;
}

Now it will handle prompting, collecting the input, and making sure it is valid.

int getCenturyValue();
int getYearValue();
int getMonthValue();

These methods do not need the "Value" suffix. getCentury, getYear, and getMonth are sufficient. Looking again, I see that these return an altered/calculated value, so the names should actually indicate that. The "Value" suffix implies this, though it's not the clearest. There is probably a better naming scheme.

int dayOfWeek();

This one, on the other hand, should have the "get" prefix for consistency: getDayOfWeek.

weekDays[date.dayOfWeek()]

Where before, the Date class had an extra responsibility, here you've externalized behavior that is perfect for inclusion into the class. We can make a getDayOfWeekName method to encapsulate this functionality.

Put it all together, and it looks like:

#include <cctype>
#include <iostream>
#include <limits>

const std::string weekDays[] = {"Sunday", "Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday"};

/**
 * The class associated with date values and calculating the day of the week
 */
class Date
{
public:
    Date(unsigned int year, unsigned int month, unsigned int day);
    int getDayOfWeek();
    std::string getDayOfWeekName(); 
    void getInput();
    bool isLeapYear();
    int getCenturyValue();
    int getYearValue();
    int getMonthValue();
private:
    unsigned int year;
    unsigned int month;
    unsigned int day;
};

/**
 * Makes sure data isn't malicious, and signals user to re-enter proper data if invalid
 */
unsigned int getSanitizedNum(std::string prompt, unsigned int min, unsigned int max)
{
    unsigned int input = 0;

    std::cout << prompt;
    while(!(std::cin >> input) || input < min || input > max)
    {
        // clear the error flag that was set so that future I/O operations will work correctly
        std::cin.clear();
        // skips to the next newline
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
        std::cout << "Invalid input. " << prompt;
    }
    return input;
}

/**
 * Safetly grabs and returns a lowercase version of the character (if the lowercase exists)
 */
char getSanitizedChar()
{
    // absorb newline character (if existant) from previous input
    if('\n' == std::cin.peek()) std::cin.ignore();
    return std::tolower(std::cin.get());
}

Date::Date(unsigned int y, unsigned int m, unsigned int d) : year(y), month(m), day(d) {
}

/**
 * Determines if the year stored in the Date instance is a leap year
 */
bool Date::isLeapYear()
{
    return (year >= 1582) && (!(year % 400) || (!(year % 4) && (year % 100)));
}

/** 
 * Finds the century, divides by 4, and returns the remainder.
 */
int Date::getCenturyValue()
{
    return (2 * (3 - div(year / 100, 4).rem));
}

/**
 * Computes a value based on the years since the beginning of the century.
 */
int Date::getYearValue()
{
    int mod = year % 100;
    return (mod + div(mod, 4).quot);
}

/**
 * Returns a value based on the given hash tables (requires invoking the isLeapYear function)
 */
int Date::getMonthValue()
{
    const static int NON_LEAP_VALS[] = { 0xDEAD, 0, 3, 3, 6, 1, 4, 6, 2, 5, 0, 3, 5 };
    const static int LEAP_VALS[] = { 0xDEAD, 6, 2, 3, 6, 1, 4, 6, 2, 5, 0, 3, 5 };
    return (isLeapYear()) ? LEAP_VALS[month] : NON_LEAP_VALS[month];
}

/**
 * Compute the sum of the date's day plus the values returned by getMonthValue, getYearValue, and getCenturyValue.
 */
int Date::getDayOfWeek()
{
    return div(day + getMonthValue() + getYearValue() + getCenturyValue(), 7).rem;
}

std::string Date::getDayOfWeekName()
{
    return weekDays[getDayOfWeek()];
}

Date getDate()
{
    unsigned int month = getSanitizedNum("Enter the month (1-12): ", 1, 12);
    unsigned int day = getSanitizedNum("Enter the day (1-31): ", 1, 31);
    unsigned int year = getSanitizedNum("Enter the year: ", 0, std::numeric_limits<unsigned int>::max());
    return new Date(year, month, day);
}

int main()
{
    do
    {
        Date date = getDate();
        std::cout << "The day of the week is " << date.getDayOfWeekName() << std::endl;

        std::cout << "Run the program again (y/N): ";  // signify n as default with capital letter
    } while ('y' == getSanitizedChar());
}

As you can see, much of it is the same, just a few tweaks. Also there were a few changes I had to make to get rid of compiler warnings (like reorder the declaration of the private members to match the order of the constructor). It also required the --std=c++11 flag to compile, so if you need it to be more portable, you'll need to remove the features that require that. Otherwise, that should do it.

Answer 2

• It seems unusual to have a member function that takes input, also considering that most of the work already done in the class is computational-based. Consider getting the input in main() and constructing a Date object from this, or overload operator>>.

• This output should be written to std::cerr instead:

  std::cout << "Invalid input.  Please enter a positive number: ";
  

  This also makes it clear to the reader that this is not a standard output, but an erroneous one.

• You have some magic numbers in isLearYear() and getCenturyValue(). Any of them can be in the respective functions as consts.

• You don't need to initialize the private variables; that's for the constructor or initializer list.

• Any member function, such as getCenturyValue(), that does not modify any data members should be const:

  int Date::getCenturyValue() const
  {
      return (2 * (3 - div(year / 100, 4).rem));
  }
  

  This not only makes the intent clearer, but also prevents any accidental modification of data members by giving a compiler error.

• You could move your two free functions next to main() to avoid possible confusion with the member functions (at least while everything exists in one file).