5
\$\begingroup\$

I am taking a beginning programming class and have been given the assignment to create a calendar as described in the title. Per the assignment the user entering offset of 0 will begin the calendar on a monday and 6 will start on sunday. There are to be 4 spaces per column and the request for input should prompt the user again if the given input isn't a valid option (ie more than 6 days or a month with less than 28 or more than 31 days).

The program works as written, what I am looking for is suggestions/critiques on ways that I might have done it better given what has been taught in the class so far. It is early in the class, so far it has been limited to pretty much if/else and loops.

#include <iostream>

int getInputDays()
{
  int inputDays = 0;
  bool validInput = false; // used to verify input is valid
  while (validInput == false)
    {
      std::cout << "Number of Days: ";
      std::cin >> inputDays;
      // will call input valid if between 28 and 31 days
      if (inputDays >= 28 && inputDays <= 31)
    validInput = true;
    }
  return inputDays;
}

int getInputOffset()
{
  int inputOffset = 0;
  bool validInput = false; // used to verify input is valid
  while (validInput == false)
    {
      std::cout << "Offset: ";
      std::cin >> inputOffset;
      // will call input valid if between 0 and 6
      if (inputOffset >= 0 && inputOffset <= 6)
    validInput = true;
    }
  // convert offset from 0 being monday to 0 being sunday
  if (inputOffset == 6)
    inputOffset = 0;
  else
    inputOffset += 1;
  return inputOffset;
}

void displayOffset(int offset)
// used to create offset on the first line of calendar
{
  if (offset == 1)
    std::cout << "       1";
  else if (offset == 2)
    std::cout << "           1";
  else if (offset == 3)
    std::cout << "              1";
  else if (offset == 4)
    std::cout << "                   1";
  else if (offset == 5)
    std::cout << "                       1";
  else
    std::cout << "                           1";
}

void drawCalendar (int days, int offset)
{
  int date = 1;
  int spaces = 0; // used to keep track of location on each line
  std::cout << "  Su  Mo  Tu  We  Th  Fr  Sa"  << std::endl;  // calendar header
  while (date <= days)
    {
      if (offset > 0)  // for months starting on any date other than sunday
    {
      displayOffset(offset);
      date = 2;
      spaces = (offset * 4) + 4;
      offset = 0;
    }
      while (spaces < 28) // 28 spaces per line per assignment
    {
      if (date >= 10) // 2 spaces before double digit numbers
        {
          if (date > days)
        break;
          std::cout << "  " << date;
        }
      else            // 3 spaces before single digit numbers
        std::cout << "   " << date;
      date += 1;
      spaces += 4;
    }
      std::cout << std::endl;
      spaces = 0; // reset spaces to 0 at the end of each line
    }
}

int main()
{
  // prompt user to input days and offset per assignment
  int days = getInputDays();
  int offset = getInputOffset();
  // use input information to create calendar display
  drawCalendar(days, offset);
}
\$\endgroup\$
2
  • \$\begingroup\$ Thank you for putting the code in the box, I'm not sure how to do that but it looks much better than it did when I pasted it. Is that something I could have done as a new user? \$\endgroup\$
    – homer150mw
    Commented Sep 4, 2016 at 1:46
  • \$\begingroup\$ Yeah, you should've. You can do it by pasting all the code, selecting it and then hitting ctrl+k. Do you know stream manipulators? I think they could greatly improve the code. I will post my answer on the next week \$\endgroup\$ Commented Sep 4, 2016 at 12:10

1 Answer 1

1
\$\begingroup\$

First Welcome to Code Review, and nice first question for a beginner. You did a good job with variable names, function names, comments and indentation, although validInput = true; is incorrectly indented in both functions. The function drawCalendar() is very inconsistent in the indentation.

Consider the User
You might want to add an error message such as "Invalid input please enter a number between X and Y" with X being the lower limit and Y being the upper limit.

DRY Principle
There is a software engineering principle call the Don't Repeat Yourself principle that may apply to the two input functions. Well written functions can be used in multiple places in code and decrease the amount of code necessary to implement a program or library. This code count contain a function called getInput() that implements most of getInputDays() and a lot of getInputOffset()

int getInput(string inputPrompt, int lowerBound, int upperBound)
{
  int inputValue = 0;
  bool validInput = false; // used to verify input is valid
  while (validInput == false)
  {
    std::cout << inputPrompt << " : ";
    std::cin >> InputValue;
    if (inputValue >= lowerBound && inputValue <= upperBound)
    {
      validInput = true;
    }
    else
    {
      std::cout << "Invalid input, please enter a number between"
          << lowerBound << " and " << upperBound << "\n";
    }
  }
  return InputValue;
}

Reducing the amount of code is a benefit because it makes it easier to write the code in the first place, modify the code later and debug the code.

You did well following another software engineering called the Single Responsibility Principle. The single responsibility principle states that every module or class should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by the class. All its services should be narrowly aligned with that responsibility.

When raw numbers are included in code they can often be confusing. Also if the same number is used multiple times in the code for the same reason, such as for array bounds it is easier to understand the code and modify the code using symbolic constants. Some examples that might apply to this program

//Symbolic constants should be capitalized, multiple words separated
// by underscore.
constexpr int SHORT_MONTH = 28;
constexpr int LONG_MONTH = 31;
constexpr int SUNDAY = 6;
constexpr int MONDAY = 0;
constexpr int TUESDAY = 1;
constexpr int SPACES_IN_COLUMN = 4;

Switch Statement Versus if then else if
The function displayOffset() could be implemented in the following manner:

void displayOffset(int offset)
// used to create offset on the first line of calendar
{
  switch (offset == 1)
    case TUESDAY :
      std::cout << "       1";
      break;
    case WEDNESDAY :
      std::cout << "           1";
      break;
   ...
  }
}

This is somewhat more readable and maintainable that multiple if then else if statements. It is much easier to add a new case than it is to add a new else if statement.

Another alternative would be:

void displayOffset(int offset)
// used to create offset on the first line of calendar
{
  for (int i = 0; i < offset * SPACES_IN_COLUMN; i++)
  {
    std::count << " ";
  }
}

Complex Functions
It might be easier to understand, debug and modify the drawCalendar() function if it was broken up into sub-functions following the Single Responsibility Principle mentioned above.

\$\endgroup\$
3
  • \$\begingroup\$ Thank you for the welcome and the feedback. I'm not sure what happened with the indentation, my code I copied from is correct but it seems to not have all copied correctly. I've read agreed in the book and seen the switch/case statements, that seems like it would be more effective that the if/else however, it hasn't been covered in class yet so I didn't use it for this assignment. For displayOffset would something like cout << " " * (4 * offset + 3) << 1; work? Then I could do it all in one line? I would try it except in not at home to try it \$\endgroup\$
    – homer150mw
    Commented Sep 4, 2016 at 2:03
  • \$\begingroup\$ You might want to look up formatting for std::cout on the internet. The question you asked me about probably won't work. Just FYI, it's not fair to you or others if I do your homework. \$\endgroup\$
    – pacmaninbw
    Commented Sep 5, 2016 at 11:10
  • \$\begingroup\$ I'm not going to submit my homework with any of the suggestions made. I posted a working program and intend to submit that. Here I am looking for ideas to improve for future use. I believe python allows using operands with strings, I don't know if C++ allows the same. \$\endgroup\$
    – homer150mw
    Commented Sep 5, 2016 at 13:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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