7
\$\begingroup\$

Here's a simple school assignment I did:

Problem 1: Write a program that asks the user for a positive integer no greater than 15. The program should then display a square on the screen using the character “X”. The number entered by the user will be the length of the side of the square. For example, if the user enters 5, the program should display the following:

XXXXX 
XXXXX 
XXXXX 
XXXXX 
XXXXX

Problem 2: Imagine that you and a number of friends go to a restaurant and when you ask for the bill you want to split the amount and the tip between all. Write a function

double CalculateAmountPerPerson(double TotalBill, double TipPercentage, int NumFriends)

that takes the total bill amount, tip percentage (e.g., 15.0 for a 15% tip), and the number of friends as inputs and returns the total bill amount as its output.

Write a main function that asks the user for the total amount of the bill and the size of his/her party (i.e., number of friends) and prints out the total amount that each person should pay for tip percentages of 10%, 12.5%, 15%, 17.5%, 20%, 22.5%, 25%, 27.5%, and 30%. Your main function should use a loop and invoke the CalculateAmountPerPerson function at each iteration.

My code:

#include <iostream>
#include <iomanip>

void menuPrompt();
short getMenuSelection();
void program1();
void squareLengthPrompt();
void displaySquare(int, char);
void program2();
void billPrompt();
void numPeoplePrompt();
double CalculateAmountPerPerson(double, double, int);
const short PROGRAM_1 = 1;
const short PROGRAM_2 = 2;
const short EXIT = 0;

int main(int argc, char *arv[]) {
    while(true) {
        char menu = getMenuSelection();
        switch (menu) {
            case EXIT:
                exit(EXIT_SUCCESS);
            case PROGRAM_1:
                program1();
                break;
            case PROGRAM_2:
                program2();
                break;
            default:
                std::cout << "That program doesn\'t exist." << std::endl;
                break;
        }
    }
    return 0;
}
void menuPrompt() {
    std::cout
        << "Menu:\n"
        << "\t1. Program 1\n"
        << "\t2. Program 2\n"
        << "\t0. Exit\n"
        << "Select your program: "
    << std::flush;
}
short getMenuSelection() {
    short selection = 0;
    while (selection != PROGRAM_1 && selection != PROGRAM_2) {
        menuPrompt();
        std::cin >> selection;
    }
    return selection;
}
void program1(){
    const char SQUARE_CHARATER = 'X';
    short squareLength = 0;
    while (squareLength > 15 || squareLength < 1) {
        squareLengthPrompt();
        std::cin >> squareLength;
    }
    displaySquare(squareLength,SQUARE_CHARATER);
}
void squareLengthPrompt() {
    std::cout
        << "Enter the length of the side of the square (Between 1 and 15): "
        << std::flush;
}
void displaySquare(int side, char character) {
    for(int i = 0; i < side ; ++i) {
        for (int j = 0; j < side; ++j) {
            std::cout << character;
        }
        std::cout << std::endl;
    }
}
void program2() {
    const float TIP_PERCENTAGES[] = {.10, .125, .15, .175, .20, .225, .25, .275, .30};
    double totalBill = 0;
    int totalPeople = 0;
    double amountPerPerson;
    while (totalBill <= 0) {
        billPrompt();
        std::cin >> totalBill;
    }
    while (totalPeople <= 0) {
        numPeoplePrompt();
        std::cin >> totalPeople;
    }
    for(auto tipPercent: TIP_PERCENTAGES){
        amountPerPerson = CalculateAmountPerPerson(totalBill,tipPercent, totalPeople);
        std::cout << "With the tip percentage of " << std::fixed << std::setprecision(2)
            << tipPercent*100 << "%, each person pays " << amountPerPerson
            << " from a $" << totalBill << " bill. " << std::endl;
    }
}
double CalculateAmountPerPerson(double TotalBill, double TipPercentage, int
                                NumFriends) {
    return (TotalBill*(1+TipPercentage))/NumFriends;
}
void billPrompt() {
    std::cout
        << "Enter the total of your bill (must be greater than 0): "
        << std::flush;
}
void numPeoplePrompt() {
    std::cout
        << "Enter the number people that are splitting the bill (must be greater than 0): "
        << std::flush;
}

I primarily just want to know if the code is self documenting and if I should include comments.

\$\endgroup\$
10
\$\begingroup\$

The code is not self-documenting, because the problem description is non-trivial and involves what you'd call "business logic". That is, you're not trying to perform some technical operation (like a cache, or a data structure, or a parser), but you're following rules defined by someone else. Each of those rules has to be programmed in, of course, but without describing WHY they have been put in, you'll always need the problem description along with the code to make sense of the code.

Imagine you had posted your question without the problem description. Would we have been able to guess what the goal of your assignment was?

Personally, yes, I think so. This is because if I were to run your program, it asks clear questions and prints a clear result. It does require a non-trivial time investment, though.

You get

"Enter the length of the side of the square (Between 1 and 15): "

As output on the screen, you enter a number, you get a square. Program 1 will print a square.

And seeing something like

for(auto tipPercent: TIP_PERCENTAGES){
    amountPerPerson = CalculateAmountPerPerson(totalBill,tipPercent, totalPeople);
    std::cout << "With the tip percentage of " << std::fixed << std::setprecision(2)
        << tipPercent*100 << "%, each person pays " << amountPerPerson
        << " from a $" << totalBill << " bill. " << std::endl;
}

In the code tells me that program 2 is for splitting up a bill.

In that sense, the code is self documenting. We don't need the problem description. We can see what the code does, because we can execute it.

What comments are for, then, are not for explaining what the code does. That understanding can already be achieved by, well, reading and executing the code.

Comments have 2 main uses, in my opinion: First, to explain the why of the code (why does the code do what it does). Second, to help speed along the understanding the code. Basically, rather than making me read and execute the entire program, spending lots of time, you simply put the purpose of a part of code in a comment, and I can read what the code does via condensed comments. Like reading a recipe instead of watching someone actually cook something.

The act of making code self-documenting, then, is to put these comments into active code. Putting the why into code is hard; the only places you can possibly do this is in error messages - "number of people must be greater than 0, cannot split bill between 0 or negative people" - stuff like that explains why there is a totalPeople <= 0 check. I don't recommend going out of your way to do that; comments are for the programmer and output is for the user.

Putting the how comments into code is a lot easier. You can use function names for this.

Compare:

    amountPerPerson = CalculateAmountPerPerson(totalBill,tipPercent, totalPeople);

and

    s = calc(sum, pct, num);

One is clear to understand, the other could mean anything. Yet it can even make sense after we give it a comment...

    double s; //share per person
    s = calc(sum, pct, num); //calculate share per person using sum costs, tip percentage and number of people

So you've, in essence, already done this "importing of comments".


There are still a few improvements to be made.

For instance...

void program2() {
    const float TIP_PERCENTAGES[] = {.10, .125, .15, .175, .20, .225, .25, .275, .30};
    double totalBill = 0;
    int totalPeople = 0;
    double amountPerPerson;
    while (totalBill <= 0) {
        billPrompt();
        std::cin >> totalBill;
    }
    while (totalPeople <= 0) {
        numPeoplePrompt();
        std::cin >> totalPeople;
    }
    for(auto tipPercent: TIP_PERCENTAGES){
        amountPerPerson = CalculateAmountPerPerson(totalBill,tipPercent, totalPeople);
        std::cout << "With the tip percentage of " << std::fixed << std::setprecision(2)
            << tipPercent*100 << "%, each person pays " << amountPerPerson
            << " from a $" << totalBill << " bill. " << std::endl;
    }
}

program2 as a whole is hard to understand. You have to carefully read what it does to see what it does. Had you instead renamed the function to runBillSplitterProgram, we'd have gotten a hint of the meaning already.

Internally, you have tried splitting certain sections up, but you're only done this for the long strings.

We can do slightly better by not separating based on code length, but on functionality:

//in runBillSplitterProgram
double amountPerPerson;
double totalBill = askForTotalBill();
int totalPeople = askForTotalPeople();

//as separate functions        
double askUserForTotalBill() {
    double totalBill = 0;
    while (totalBill <= 0) {
        std::cout
            << "Enter the total of your bill (must be greater than 0): "
            << std::flush;
        std::cin >> totalBill;
    }
    return totalBill;
}

int askUserForTotalPeople() {
    int totalPeople = 0;
    while (totalPeople <= 0) {
        std::cout
            << "Enter the number people that are splitting the bill (must be greater than 0): "
            << std::flush;
        std::cin >> totalPeople;
    }
    return totalPeople;
}

It's a shame one is a double and the other is an integer, or you'd been able to merge both into some sort of askUserForValue function, keeping billPrompt as a function which calls askUserForValue with a lengthy string.


Technical commentary:

while (!(1 <= squareLength && squareLength <= 15))

reads better as a range check.


Overall, you've done a pretty good job. You should add a program header describing the exercise.

\$\endgroup\$
  • 2
    \$\begingroup\$ "It's a shame one is a double and the other is an integer, or you'd been able to merge both" you can merge them with generics i think. \$\endgroup\$ – Caridorc Jul 22 '16 at 13:37
  • 1
    \$\begingroup\$ @Caridorc That would be a great answer; but it's not really in scope of my answer as I treat the asker as a bit of a beginner - handing in a solution with generics might be a bit over the top for a task such as this \$\endgroup\$ – Pimgd Jul 22 '16 at 13:40
  • 1
    \$\begingroup\$ Well, I really try to avoid code cuplication even if the cost in complexity is big. Repeated code means that (the subset of) the language you are using is not abstract enough. \$\endgroup\$ – Caridorc Jul 22 '16 at 13:42
  • 1
    \$\begingroup\$ And generics are really not that hard (it is like formal parameters for types instead of values (first class types)) \$\endgroup\$ – Caridorc Jul 22 '16 at 13:44
  • 1
    \$\begingroup\$ @Caridorc I'm suggesting that you post it as a separate answer. \$\endgroup\$ – Pimgd Jul 22 '16 at 13:44
6
\$\begingroup\$

@Pimgd has covered the readability of your code, so I just have a few points:

  • You can't currently exit your program with the menu. Although your menu contains '0. Exit', getMenuSelection won't return unless you pick PROGRAM_1 or PROGRAM_2.
  • SQUARE_CHARATER should probably be SQUARE_CHARACTER
  • Consider testing edge cases. Your code rounds down the tip. This means it's possible to achieve a 'you don't have to pay the bill' situation (which obviously wouldn't happen in real life):

    Enter the total of your bill (must be greater than 0): .03
    Enter the number people that are splitting the bill (must be greater than 0): 7
    With the tip percentage of 10.00%, each person pays 0.00 from a $0.03 bill.
    
  • Consider what happens if you type one for the number of people (or 0.1) etc.

\$\endgroup\$
  • \$\begingroup\$ Thank you! This is really interesting, I guess I need to be aware that any that reduces precision like std::setprecision(2) will introduce edge cases of rounding issues. Something I will definitely keep in mind forever. \$\endgroup\$ – Harrison Tran Jul 23 '16 at 0:08
  • \$\begingroup\$ Also I just entered .1 into the program and it produced a infinite loop, could you explain whats happening? \$\endgroup\$ – Harrison Tran Jul 23 '16 at 0:28
  • \$\begingroup\$ @HarrisonTran read: stackoverflow.com/a/10349885/592182 \$\endgroup\$ – forsvarir Jul 23 '16 at 3:43

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.