1
\$\begingroup\$

My first program using Classes, it should allow you to set the price of three different pumps, times how long you pump, then calculates what you owe, and updates the amount of gas left in the pump. Working on an associates degree and my teacher doesn't give any critique so anything you all could point out would be helpful and appreciated. Here it is...

#include <iostream>
#include <iomanip>
#include <string>
#include <ctime> //for the timer
#include <cctype> //toupper and to lower functions
using namespace std;

const float FLOWRATE = .18;
const float MAX_GAS = 100.0;

class GasPump {
public:
    float RegularInPump(float rip);
    float SuperInPump(float sip);
    float DiesalInPump(float dip);
    void SetPrice();
    void DisplayOutput(int gasChoice, float amountDispensed, float &regularInPump, float &superInPump, float &diesalInPump);

private:
    float regularInPump;
    float superInPump;
    float diesalInPump;
    float regGasPrice;
    float supGasPrice;
    float dieselGasPrice;
};

float TimeSpentPumping(float &timePumping, float &amountDispensed, int &gasChoice) {
    while (true) {
        cout << "Do you want unleaded, super unleaded or diseal gas?(Enter 1, 2, or 3) ";
        cin >> gasChoice;

        if ((cin) && (gasChoice >= 1 && gasChoice <= 3)) {
            cin.ignore();
            break;
        }
    }

    //Start pump timer
    cout << "Press Enter to start pumping.";
    cin.ignore();
    float start = clock();

    //stop pump timer
    cout << "Press Enter when done pumping.";
    cin.ignore();
    timePumping = (clock() - start) / 1000;
    cout << endl;

    cout << "You pumped for " << timePumping << " seconds." << endl;
    cout << endl;
    amountDispensed = timePumping * FLOWRATE;

    return amountDispensed;
}

int RestartProgram() {
    char response;
    bool endProgram = false;

    cout << std::endl;
    while (true) {
        cout << "Do you wish to run this program again? Enter Y or N: ";
        cin >> response;
        response = tolower(response);
        if ((cin) && response == 'y' || response == 'n') {
            cin.ignore();
            break;
        }
    }

    if (response == 'n') {
        return true;
    }

    return 0;
}

int main()
{
    GasPump SetPump;
    float timePumping = 0.0;
    float amountDispensed = 0.0;
    float regularInPump = MAX_GAS;
    float superInPump = MAX_GAS;
    float diesalInPump = MAX_GAS;
    int gasChoice = 0; //1=reg, 2=super, 3=diseal
    bool endProgram = 0;

    cout.setf(ios::fixed);
    cout.setf(ios::showpoint);
    cout.precision(2);

    //Set intial pump prices
    SetPump.SetPrice();

    while (!endProgram) {
        amountDispensed = TimeSpentPumping(timePumping, amountDispensed, gasChoice);

        SetPump.DisplayOutput(gasChoice, amountDispensed, regularInPump, superInPump, diesalInPump);

        endProgram = RestartProgram();
    }
    return 0;
}

void GasPump::SetPrice() { 
    cout << "Enter the price for unleaded gas: ";
    cin >> regGasPrice;

    cout << "Enter the price for super unleaded gas: ";
    cin >> supGasPrice;

    cout << "Enter the price for diesal gas: ";
    cin >> dieselGasPrice;
    cin.ignore();

}

void GasPump::DisplayOutput(int gasChoice, float amountDispensed, float &regularInPump, float &superInPump, float &diesalInPump) {
    float amtOwed = 0.0;
    float pumpPrice = 0.0;
    float pumpLevel = 0.0;
    string gasString;

    //Calculate price of chosen gas
    switch (gasChoice) {
    case 1:
        pumpPrice = regGasPrice;
        amtOwed = amountDispensed * regGasPrice;
        pumpLevel = RegularInPump(regularInPump -= amountDispensed);
        gasString = "Regular Gas";
        break;
    case 2:
        pumpPrice = supGasPrice;
        amtOwed = amountDispensed * supGasPrice;
        pumpLevel = SuperInPump(superInPump -= amountDispensed);
        gasString = "Super Unleaded Gas";
        break;
    default:
        pumpPrice = dieselGasPrice;
        amtOwed = amountDispensed * dieselGasPrice;
        pumpLevel = DiesalInPump(diesalInPump -= amountDispensed);
        gasString = "Diesal Gas";
        break;
    }

    //Display results
    int indent = 20;
    cout << setw(indent) << left << "Amount dispensed: " << amountDispensed << " gallons of " << gasString << endl;
    cout << setw(indent) << left << "At a price of: " << "$" << pumpPrice << endl;
    cout << setw(indent) << left << "You owe " << "$" << amtOwed << endl;
    cout << endl;
    cout << "There is " << pumpLevel << " gallons of " << gasString << " in the pump." << endl;
    cout << endl;
}

float GasPump::RegularInPump(float rip) {
    regularInPump = rip;
    return regularInPump;
};

float GasPump::SuperInPump(float sip) {
    superInPump = sip;
    return superInPump;
};

float GasPump::DiesalInPump(float dip) {
    diesalInPump = dip;
    return diesalInPump;
};
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Using namespace std pollutes the global namespace. It's better to use everything with its full name (std::vector, std::cout, etc...)

using namespace std; //this is bad

In int RestartProgram(), the return type is int, but you return true and 0. Just return a bool and return true or false. You also never use the endProgram variable. Also, && comes before ||, and you probably meant to surround the response checks with parentheses:

bool RestartProgram() {
    char response;

    std::cout << std::endl;
    while (true) {
        std::cout << "Do you wish to run this program again? Enter Y or N: ";
        std::cin >> response;
        response = tolower(response);
        if (std::cin && (response == 'y' || response == 'n')) {
            std::cin.ignore();
            break;
        }
    }

    return response == 'n';
}

It's also a little confusing that you return false from RestartProgram to indicate that you want to keep running the program. I would probably change the name to ContinueProgram and return true to indicate that you want to continue.

Float literals have an f on the end of them:

const float FLOWRATE = .18f;
const float MAX_GAS = 100.0f;

And use true and false for bool. To fix the literals in main:

float timePumping = 0.0f;
float amountDispensed = 0.0f;
//...
bool endProgram = false; //false instead of 0

There's not much point in both passing amountDispensed as reference and also returning it.

float TimeSpentPumping(float &timePumping, float &amountDispensed, int &gasChoice) {
    //.. comment added by me... the only time the variable is ever used.
    amountDispensed = timePumping * FLOWRATE;

    return amountDispensed;
}

Just pick one. Here I just return void, keeping with the semantics of the rest of the parameters (pass by non-const reference):

void TimeSpentPumping(float &timePumping, float &amountDispensed, int &gasChoice) {
    //.. comment added by me... the only time the variable is ever used.
    amountDispensed = timePumping * FLOWRATE;
}

Setters usually start with set, and even more often have a void return:

void SetRegularInPump(float rip);
void SetSuperInPump(float sip);
void SetDieselInPump(float dip);

void GasPump::SetRegularInPump(float rip) {
    regularInPump = rip;
};

void GasPump::SetSuperInPump(float sip) {
    superInPump = sip;
};

void GasPump::SetDieselInPump(float dip) {
    dieselInPump = dip;
};

Of course, then you have to fix the other parts of your program that use them. In addition, I'd stop doing assignments inside of parameter lists and if and while headers. It's dangerous to do so, especially as a beginner. Here I've changed the first case:

switch (gasChoice) {
case 1:
    pumpPrice = regGasPrice;
    regularInPump -= amountDispensed;
    pumpLevel = regularInPump;
    SetRegularInPump(pumpLevel);
    gasString = "Regular Gas";
    break;
//...
}

//And you can do the amtOwed calculation outside the switch:
amtOwed = amountDispensed * pumpPrice;

Consistency is important. The rest of your variables start with a lowercase letter, so name the GasPump variable in the same way:

GasPump setPump; //although I'd probably just name it "pump"

Use const when you can. It lets the programmer know (reasonably well) at a glance that a variable will never be modified again. The start variable can be declared as such since it's never changed. Also, clock returns a clock_t, so I'd probably declare it as such and cast later:

const std::clock_t start = clock();

Also, you probably want to use CLOCKS_PER_SEC, not 1000:

timePumping = (clock() - start) / (float)CLOCKS_PER_SEC;

I don't know if this is on topic, but "diesel" is misspelled as "diesal".

\$\endgroup\$
2
  • \$\begingroup\$ Thanks so much for the response! I used to not use namespace, but my teacher this semester told me to. I'm gonna go back to not using it. I used your suggestions in the Restart function too. I usually just copy and paste it in from a notepad file I keep of handy code and I haven't really looked at it since I wrote it. I see what you're saying with naming setters with Set, I'll start remembering that. I haven't learned about clocks in class, I just tried that from googling so I really appreciate your tips there. Frustrating that I got a 100 on this, I learn more from this site than I do school. \$\endgroup\$
    – Sean
    Commented Nov 11, 2017 at 0:03
  • 1
    \$\begingroup\$ @Sean You're welcome. Sorry for the late response here. I'm glad my answer helped. There are certainly many people who graduate without knowing how to code, but I think a 100 is fair here. You're clearly learning and students can't be expected to write anything close to clean, maintainable code. \$\endgroup\$ Commented Dec 1, 2017 at 6:34

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.