11
\$\begingroup\$

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());
}
\$\endgroup\$
7
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Two minor nitpicks: The "Value" prefix is actually a postfix/suffix. The new getSanitizedNum function should probably be named getSanitizedInteger and the function could automatically print the valid range so the user (of the function) does not have to do it. (It could also be made mutually exclusive: Give the prompt parameter a default (empty) and if it is not empty the prompt will be printed, else getSanitizedNum will create one on its own.) \$\endgroup\$ – Nobody Dec 11 '14 at 9:30
  • \$\begingroup\$ @Nobody Re prefix/suffix, you are right. I have corrected it. Re getSanitizedNum function, I agree with you there. I had considered doing both of those things. The idea of auto-generating one is also pretty good. \$\endgroup\$ – cbojar Dec 11 '14 at 13:54
9
\$\begingroup\$
  • 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).

\$\endgroup\$
  • \$\begingroup\$ Nice point about std::cerr, one could go even further and say: std::cerr/cout/cin are global variables and should be avoided (instead pass them as parameters) but in such simple cases this might be overkill. \$\endgroup\$ – Nobody Dec 11 '14 at 9:31

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.