Ticket sale income calculator

I am looking for some useful feedback on my programming. I'm only three days in and trying to improve.

Here is the code:

```
#include
using namespace std;
const double SECT_A_TICKPRICE = 20;
const double SECT_B_TICKPRICE = 15;
const double SECT_C_TICKPRICE = 10;

double getSeatsA();
double getSeatsB();
double getSeatsC();
double calcIncomeA(double);
double calcIncomeB(double);
double calcIncomeC(double);
void showIncome(double, double, double);

int main()
{
double aSeats;
double bSeats;
double cSeats;
double incomeSectA;
double incomeSectB;
double incomeSectC;

aSeats = getSeatsA();

bSeats = getSeatsB();

cSeats = getSeatsC();

incomeSectA = calcIncomeA(aSeats);

incomeSectB = calcIncomeB(bSeats);

incomeSectC = calcIncomeC(cSeats);

showIncome(incomeSectA, incomeSectB, incomeSectC);

return 0;

}

double geatSeatsA()
{
double totalSeatsA;

cout > totalSeatsA;

while (totalSeatsA 300)
{
cout > totalSeatsA;
}

return totalSeatsA;
}

double geatSeatsB()
{
double totalSeatsB;

cout > totalSeatsB;

while (totalSeatsB 500)
{
cout > totalSeatsB;
}

return totalSeatsB;
}

double geatSeatsC()
{
double totalSeatsC;

cout > totalSeatsC;

while (totalSeatsC 200)
{
cout > totalSeatsC;
}

return totalSeatsC;
}

double calcIncomeA(double aSeats)
{
double incomeA;

incomeA = aSeats * SECT_A_TICKPRICE;

return incomeA;
}

double calcIncomeB(double bSeats)
{
double incomeB;

incomeB = bSeats * SECT_B_TICKPRICE;

return incomeB;
}

double calcIncomeC(double cSeats)
{
double incomeC;

incomeC = cSeats * SECT_C_TICKPRICE;

return incomeC;
}

void showIncome(double incomeSectA, double incomeSectB, double incomeSectC)
{
double allincome;

allincome = incomeSectA + incomeSectB + incomeSectC;

cout << "The income for Section A is ";

• Don't use floating point numbers for representing money.

• Don't using namespace std, it's a bad habit that will bite you down the line.

• You've "double-spaced" pretty much all your code. That's too much vertical whitespace.

• Try to declare your variables as late as possible, ideally only when you can initialize them. This avoids bugs linked to using uninitialized variables, and keeps their scope as small as possible which makes the code simpler to follow - less state to keep in mind. Don't declare variables then initialize them one line after, just do both at once.

Example for this last point:

double incomeB;

incomeB = bSeats * SECT_B_TICKPRICE;

return incomeB;

Should be:

double incomeB = bSeats * SECT_B_TICKPRICE;
return incomeB;

Or better simply:

return bSeats * SECT_B_TICKPRICE;

Also:

double aSeats;
double bSeats;
double cSeats;

aSeats = getSeatsA();

bSeats = getSeatsB();

cSeats = getSeatsC();

Should be:

double aSeats = getSeatsA();
double bSeats = getSeatsB();
double cSeats = getSeatsC();

For the first point about money, simplest way out is to use int and compute all prices in cents. You can split out dollars and cents (or euros, whatever) on display. I'll use that for the rest of this review.

Also I doubt that buying 0.42 seats makes much sense, so the number of seats purchased/booked should use integers too.

Then look at your repeated functions, and look at what differences they have. Then try to make a more generic function, often by passing additional parameters.

Your getSeatsX functions only depend on a section name, and a number of available seats for that section. So put those in parameters.

int getSeatsForSection(std::string const& name, int maxSeats)
{
    std::cout > seats;

    while (seats  maxSeats)
    {
        std::cout > seats;
    }

    return seats;
}

Your calcIncomeX functions only depend on the price of the seats in that section, so move that to a parameter too.

int calcIncome(int seats, int centsPerSeat)
{
    return seats * centsPerSeat;
}

So your main becomes:

const int SECT_A_TICKPRICE = 2000; // price in cents
// ...
int main()
{
  int seatsA = getSeatsForSection("A", 300);
  // ...
  int incomeA = calcIncome(seatsA, SECT_A_TICKPRICE);
  // ...
}

With a little helper to display dollar amounts, you can update your showIncome function easily:

void displayIncome(int cents)
{
  std::cout << (cents / 100) << " dollars and "
            << (cents % 100) << " cents" << std::endl;
}

(Or use a string stream and return the formatted number, either like above or in xxx.yy format.)

But that's still not very good. If you need to add a new section, you need to add a bunch of code in several different places.

I'd suggest the following:

• Create a struct that represents sections. It should contain the section name, total number of seats and seat price.

• Build a std::vector of all your sections.

• Use that to loop over all sections for input and display.

Rough sketch of what main would look like with that:

int main()
{
  std::vector sections = { /* ... */ };
  int totalIncome = 0;

  for (auto const& sect : sections) {
    int seats = getSeatsForSection(sect.name, sect.maxSeats);
    int income = calcIncome(seats, sect.seatPrice);
    displayIncomeForSection(sect.name, income);
    totalIncome += income;
  }
  displayTotalIncome(totalIncome);
}

There are lots of other options possible, depending on exactly what you need to do. This is just a suggestion.

StackExchange Code Review Q#124802, answer score: 4