3
\$\begingroup\$

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 <iostream>
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 << "How many tickets were sold in Section A?" << endl;

    cin >> totalSeatsA;

        while (totalSeatsA < 0 && totalSeatsA > 300)
        {
            cout << "Enter a value between 0 and 300" << endl;

            cin >> totalSeatsA;
        }

    return totalSeatsA;
}

double geatSeatsB()
{
    double totalSeatsB;

    cout << "How many tickets were sold in Section B?" << endl;

    cin >> totalSeatsB;

        while (totalSeatsB < 0 && totalSeatsB > 500)
        {
            cout << "Enter a value between 0 and 500" << endl;

            cin >> totalSeatsB;
        }

    return totalSeatsB;
}

double geatSeatsC()
{
    double totalSeatsC;

    cout << "How many tickets were sold in Section C?" << endl;

    cin >> totalSeatsC;

        while (totalSeatsC < 0 && totalSeatsC > 200)
        {
            cout << "Enter a value between 0 and 200" << endl;

            cin >> 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 ";

    cout << incomeSectA << endl;

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

    cout << incomeSectB << endl;

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

    cout << incomeSectC << endl;

    cout << "The total amount made from ticket sales is ";

    cout << allincome;

}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. \$\endgroup\$
    – Dan
    Apr 5 '16 at 7:41
4
\$\begingroup\$
  • 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 << "How many tickets were sold in Section " 
              << name << "?" << std::endl;

    int seats;
    std::cin >> seats;

    while (seats < 0 && seats > maxSeats)
    {
        std::cout << "Enter a value between 0 and "
                  << maxSeats << std::endl;
        std::cin >> 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<section> 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.

\$\endgroup\$

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.