1
\$\begingroup\$

I was wondering if anyone would be kind enough to review my basic c++ program. I'm a beginner programmer so was hoping to get feedback on how well I've used object oriented programming i.e. using a class in my program. And in general, how can I make my program better? Many thanks in advance.

//  Created by George Austin Bradley on 08/12/2019.
//  Copyright © 2019 George Austin Bradley. All rights reserved.
//

#include <iostream>
#include <string>
#include <vector>
#include <iomanip>
using namespace std;

class User{
private:
    string sName;
    string sCarName;
    string sCarFuelType;
    string sChoiceOfLocation;
    double dFuelCost;
    int iMilesToTravel;
    double dTotalToBePaid;
    double dVATAmount;
    double dEstimatedTime;
public:
    User()
    :sName(""),sCarName(""), sCarFuelType(""), sChoiceOfLocation(""), dFuelCost(0), iMilesToTravel(0), dTotalToBePaid(0),dVATAmount(0), dEstimatedTime(0){
    }

    void SetName()
    {
        cout << "What's your name?: ";
        cin >> sName;
    }

    void SetCarFuelType(){
        bool bValid = false;
        char cSelection = 0;
        do{
            cout << "What's your fuel type? (p) petrol or (d) diesel: ";
            cin >> cSelection;
            cSelection = toupper(cSelection);
            if (cSelection == 'P')
            {
                sCarFuelType = "Petrol";
                dFuelCost = 0.33;
                cout << "You've chosen " << sCarFuelType << " as your fuel type.\n";
                bValid = true;
            }
            else if (cSelection == 'D')
            {
                sCarFuelType = "Diesel";
                dFuelCost = 0.40;
                cout << "You've chosen " << sCarFuelType << " as your fuel type.\n";
                bValid = true;
            }
            else
            {
                cout << "Invalid input. Please try again!";
            }
        }while(bValid == false);
    }
    void SetCarName()
    {

        bool bValid = false;
        do{
            cout << "What car do you drive?: ";
            cin >> sCarName;
            bValid = true;

        }while(bValid == false);

    }


    void DisplayTravelDestinations()
    {
        cout << "\n";
        cout << "Select a location number from the menu...\n";
        cout << "1. South West - 141 Miles\n";
        cout << "2. Leeds - 195\n";
        cout << "3. Birmingham - 220\n";
        cout << "4. Glasglow - 230 miles\n";
        cout << "\n";
    }
    void SetLocation(){
        bool bValid = false;
        char cSelection = 0;
        do{
            DisplayTravelDestinations();
            cout << "Where do you want to travel?: ";
            cin >> cSelection;
            cSelection = toupper(cSelection);
            switch(cSelection)
            {
                case '1':
                    sChoiceOfLocation = "South West";
                    iMilesToTravel = 141;

                    bValid = true;
                    break;
                case '2':
                    sChoiceOfLocation = "Leeds";
                    iMilesToTravel = 195;
                    bValid = true;
                    break;
                case '3':
                    sChoiceOfLocation = "Birmingham";
                    iMilesToTravel = 220;
                    bValid = true;
                    break;
                case '4':
                    sChoiceOfLocation = "Glasglow";
                    iMilesToTravel = 230;
                    bValid = true;
                    break;
                default:
                    cout << "Invalid input! Please try again.";
                    continue;
            }
        }while(bValid == false);

    }
    void CalculateTotalToBePaid()
    {
        if (iMilesToTravel < 100)
        {
            dTotalToBePaid = iMilesToTravel * dFuelCost;

        }
        else
        {
            dTotalToBePaid = 100 * dFuelCost;
            dTotalToBePaid += (iMilesToTravel - 100) * (dFuelCost / 2);
        }
    }

    void CalculateVAT()
    {
        double dVATPercentage = 0.2;
        dVATAmount = dTotalToBePaid * dVATPercentage;

    }
    void CalculateEstimatedTravelTime()
    {
        const int iAverage = 50;
        dEstimatedTime = iMilesToTravel / iAverage;
    }
    string GetName(){return sName;}
    string GetCarName(){return sCarName;}
    string GetFuelType(){return sCarFuelType;}
    string GetChoiceOfLocation(){return sChoiceOfLocation;}
    double GetFuelCost(){return dFuelCost;}
    int GetMilesToTravel(){return iMilesToTravel;}
    double GetTotalToBePaid(){return dTotalToBePaid;}
    double GetVATAmount(){return dVATAmount;}
    double GetEstimatedTime(){return dEstimatedTime;}

};



bool ContinueOptions()
{
    char cSelection = 0;
    cout << "Do you wish to restart? (Y/N) ";
    cin >> cSelection;
    cSelection = toupper(cSelection);
    if(!cin)
    {
        return false;
    }
    bool bGoAgain = toupper(static_cast<unsigned char>(cSelection)) == 'Y';
    cout << (bGoAgain ? "You've chosen to start again\n" : "Goodbye!\n");
    return bGoAgain;
}

void DisplayResults(User George)
{
    cout << fixed << setprecision(2);
    cout << "Hi " << George.GetName() << ", you're driving in a " << George.GetCarName() << ".\n";
    cout << "The car your driving runs on " << George.GetFuelType() << " which costs £" << George.GetFuelCost() << " an hour.\n";
    cout << "The destination you have chosen is " << George.GetChoiceOfLocation() << " and is a distance of " << George.GetMilesToTravel() << " miles.\n";
    cout << "The journey will cost you £" << George.GetTotalToBePaid() << " with 20% VAT: £" << George.GetVATAmount() << ".\n";
    cout << "The estimated amount of time it will take is " << George.GetEstimatedTime() << " hours.\n";
}

void GetUserInformation(User &George)
{
    George.SetName();
    George.SetCarName();
    George.SetCarFuelType();
    George.SetLocation();
    George.CalculateTotalToBePaid();
    George.CalculateVAT();
    George.CalculateEstimatedTravelTime();
}

int main() {

    do{
        User George;
        GetUserInformation(George);
        DisplayResults(George);
    }while(ContinueOptions());

    return 0;
}
\$\endgroup\$
1
  • \$\begingroup\$ You might want to consider following the advice of previous questions in new questions, the code still has using namespace std; in it. You might also want to remove the licensing information since posting the code here effectively makes in public domain and Stack Overflow Inc. Has their own licensing for the questions asked. \$\endgroup\$
    – pacmaninbw
    Dec 23, 2019 at 22:57

1 Answer 1

4
\$\begingroup\$

Avoid using namespace std;

If you are coding professionally you probably should get out of the habit of using the using namespace std; statement. The code will more clearly define where cout and other identifiers are coming from (std::cin, std::cout). As you start using namespaces in your code it is better to identify where each function comes from because there may be function name collisions from different namespaces. The identifiercout you may override within your own classes, and you may override the operator << in your own classes as well. This stack overflow question discusses this in more detail.

DRY Code

There is a programming principle called the Don't Repeat Yourself Principle sometimes referred to as DRY code. If you find yourself repeating the same code multiple times it is better to encapsulate it in a function. If it is possible to loop through the code that can reduce repetition as well.

In the example code below, 2 private functions, SetExcusionData(std::string location, int distance) and SetFuelTypeAndCost(std::string ftype, double cost) have been added. These private functions reduce the code in SetLocation() and SetCarFuelType().

The logic in SetLocation() has been altered slightly to reduce the code in the function as well, the variable bValid is initialized to true rather than false, and the only place bValid needs to be modified is in the default case. There is no reason to use toupper() in this function because there is no upper case 1 through 4. The logic has also been modified so that the variable cSelection is an integer rather than a character.

Readability

The code would be more readable if there was more horizontal spacing, int the code most of the do while loops end with something like this }while(bValid == false); there should space between } and while and there should be a space between while and (.

Minor Input Issue

I generally include the make and model of my car as a car name, the code currently doesn't handle multiple word line well as input, it might be better in some cases to get the whole line of input and process it. I might also want to use both my first and last name when asked my name.

Program Organization

Most classes in C++ are implemented by a header file and a c++ source file. In most C++ editors there should be a button or a menu item that supports Add Class that creates this two file structure for you. One of the major benefits of this organization is that functions can be maintained/edits without needed to rebuild other files that include the header. Another benefit of this file organization is it somewhat easier to read the definition of the class, since the member variables and the member functions are just a list. Here is the User class broken up using a normal C++ editor:

User2.h

#ifndef CARMILAGEGEORGE_USER2_H
#define CARMILAGEGEORGE_USER2_H

#include <iostream>
#include <string>
#include <vector>
#include <iomanip>    

class User2 {
public:
    User2();
    void SetName();
    void SetCarFuelType();
    void SetCarName();
    void DisplayTravelDestinations();
    void SetLocation();
    void CalculateTotalToBePaid();
    void CalculateVAT();
    void CalculateEstimatedTravelTime();
    std::string  GetName(){return sName;}
    std::string  GetCarName(){return sCarName;}
    std::string  GetFuelType(){return sCarFuelType;}
    std::string  GetChoiceOfLocation(){return sChoiceOfLocation;}
    double GetFuelCost(){return dFuelCost;}
    int GetMilesToTravel(){return iMilesToTravel;}
    double GetTotalToBePaid(){return dTotalToBePaid;}
    double GetVATAmount(){return dVATAmount;}
    double GetEstimatedTime(){return dEstimatedTime;}

private:
    void SetFuelTypeAndCost(std::string ftype, double cost);
    void SetExcusionData(std::string location, int distance);
    std::string  sName;
    std::string  sCarName;
    std::string  sCarFuelType;
    std::string  sChoiceOfLocation;
    double dFuelCost;
    int iMilesToTravel;
    double dTotalToBePaid;
    double dVATAmount;
    double dEstimatedTime;
};

#endif //CARMILAGEGEORGE_USER2_H

User2.cpp

#include "User2.h"

User2::User2()
    :sName(""), sCarName(""), sCarFuelType(""), sChoiceOfLocation(""), dFuelCost(0), iMilesToTravel(0), dTotalToBePaid(0),dVATAmount(0), dEstimatedTime(0)
{
}

void User2:: SetName()
{
    std::cout <<  "What's your name?: ";
    std::cin >> sName;
}

void User2::SetFuelTypeAndCost(std::string ftype, double cost)
{
    sCarFuelType = ftype;
    dFuelCost = cost;
}

void User2:: SetCarFuelType(){
    bool bValid = false;
    char cSelection = 0;
    std::cout <<  "What's your fuel type? (p) petrol or (d) diesel: ";

    do{
        std::cin >> cSelection;
        cSelection = toupper(cSelection);
        if (cSelection == 'P')
        {
            SetFuelTypeAndCost("Petrol", 0.33);
            bValid = true;
        }
        else if (cSelection == 'D')
        {
            SetFuelTypeAndCost("Diesel", 0.40);
            bValid = true;
        }
        if (!bValid)
        {
            std::cout <<  "Invalid input. Please try again!";
        }
    } while (bValid == false);

    std::cout <<  "You've chosen " << sCarFuelType << " as your fuel type.\n";
}

void User2::SetCarName()
{
    bool bValid = false;

    do{
        std::cout <<  "What car do you drive?: ";
        std::cin >> sCarName;
        bValid = true;

    } while (bValid == false);

}

void User2::DisplayTravelDestinations()
{
    std::cout <<  "\n";
    std::cout <<  "Select a location number from the menu...\n";
    std::cout <<  "1. South West - 141 Miles\n";
    std::cout <<  "2. Leeds - 195\n";
    std::cout <<  "3. Birmingham - 220\n";
    std::cout <<  "4. Glasglow - 230 miles\n";
    std::cout <<  "\n";
}

void User2::SetExcusionData(std::string location, int distance)
{
    sChoiceOfLocation = location;
    iMilesToTravel = distance;
}

void User2::SetLocation(){
    bool bValid = true;
    int cSelection = 0;
    do{
        DisplayTravelDestinations();
        std::cout <<  "Where do you want to travel?: ";
        std::cin >> cSelection;
        switch(cSelection)
        {
            case 1:
                SetExcusionData("South West", 141);
                break;
            case 2:
                SetExcusionData("Leeds", 195);
                break;
            case 3:
                SetExcusionData("Birmingham", 220);
                break;
            case 4:
                SetExcusionData("Glasglow", 230);
                break;
            default:
                std::cout <<  "Invalid input! Please try again.";
                bValid = false;
                continue;
        }
    } while (bValid == false);

}

void User2::CalculateTotalToBePaid()
{
    if (iMilesToTravel < 100)
    {
        dTotalToBePaid = iMilesToTravel * dFuelCost;

    }
    else
    {
        dTotalToBePaid = 100 * dFuelCost;
        dTotalToBePaid += (iMilesToTravel - 100) * (dFuelCost / 2);
    }
}

void User2::CalculateVAT()
{
    double dVATPercentage = 0.2;
    dVATAmount = dTotalToBePaid * dVATPercentage;

}

void User2::CalculateEstimatedTravelTime()
{
    const int iAverage = 50;
    dEstimatedTime = iMilesToTravel / iAverage;
}
\$\endgroup\$
7
  • \$\begingroup\$ Hi, thanks so much for reviewing my program. I really appreciate how you’ve shown me how to separate the class into a different file. And what you did with the duplicated code is just fantastic and has made things even better in terms of efficiency. I shall definitely follow that rule of thumb for future reference. Many thanks! \$\endgroup\$ Dec 23, 2019 at 23:45
  • \$\begingroup\$ I have a question. Is there a better way to input my calculations instead of calling them in a procedure outside the class? If you know what I mean. \$\endgroup\$ Dec 24, 2019 at 11:59
  • \$\begingroup\$ Rather then calling all three calculations in void GetUserInformation(User2 &George) there could be a single public interface called Calculate that called the 3 calculation functions. The other option is that the constructor doesn't do very much right now and the body of void GetUserInformation(User2 &George) could all be in the constructor. \$\endgroup\$
    – pacmaninbw
    Dec 24, 2019 at 13:50
  • \$\begingroup\$ The function DisplayResults(User2 George) should be in the class User as well, that would remove the need for public accessor functions. You could actually make DisplayResults(User2 George) into an override of the << operator for User. std::cout << George;. Another reason not to use using namespace std;. \$\endgroup\$
    – pacmaninbw
    Dec 24, 2019 at 14:00
  • \$\begingroup\$ How would I make DisplayResults(User2 George) into an override operator? \$\endgroup\$ Dec 24, 2019 at 16:47

Your Answer

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

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