First OOP in C++ - Car example

Question

A code challenge between some friends, as I came from a Procedural way of coding, instead of using Objects. The paramaters are below:

• store the properties of a car
  • colour
  • make
  • model
  • speed
  • direction
• demonstrate how these properties are accessed
• manipulate these properties through standard calls (functions/methods)
  • from rest, accelerate to 60km/h
  • make a 90-degree left-hand turn
  • make a 90-degree right-hand turn
  • accelerate to 90km/h
  • brake to 0km/h

I am not used to C++, but decided to try it with some tutorials. I do not think I'm doing it correctly though, looking at other examples. Am I demonstrating how the properties are accessed properly? Am I using the getters and setters correctly?

main.cpp

#include <iostream>
#include "car.h"

using namespace std;
int main()
{
    car MyCar;
    int randomCrapIDontCareAbout;
    cout << "OOP Code Challenge - Cars" << endl;

    MyCar.SetColour(4); // 1-4 only
    MyCar.SetMake(3); // 1-4 only
    MyCar.SetDirection(2); // 1-4 only
    MyCar.SetModel(1); // 1-4 only
    MyCar.SetSpeed(0); // 1-200 only

    cout << "Initial Values:" << endl;
    MyCar.PrintCurrentCar();

    cout << "Step 1 - From rest, accelerate to 60km/h"<<endl;
    MyCar.SetSpeed(60);
    MyCar.PrintCurrentCar();

    cout << "Step 2 - Make a 90-degree left-hand turn" << endl;
    MyCar.ChangeDirection(-1);
    MyCar.PrintCurrentCar();

    cout << "Step 3 - Make a 90-degree right-hand turn" << endl;
    MyCar.ChangeDirection(+1);
    MyCar.PrintCurrentCar();

    cout << "Step 4 - Accelerate to 90km/h" << endl;
    MyCar.SetSpeed(90);
    MyCar.PrintCurrentCar();

    cout << "Step 5 - Brake to 0km/h" << endl;
    MyCar.SetSpeed(0);
    MyCar.PrintCurrentCar();

    cin >> randomCrapIDontCareAbout;
    return 0;
}

car.cpp

#include "car.h"

car::car()
{
    //ctor
    SetColour(1);
    SetDirection(1);
    SetMake(1);
    SetModel(1);
}

car::~car()
{
    //dtor
}

car.h

#ifndef CAR_H
#define CAR_H
#include <iostream>

using namespace std;

class car
{
    public:
        car();
        virtual ~car();
        void PrintCurrentCar() {
            cout << "COLOUR:     ";
            GetColour();
            cout << endl << "MAKE:       ";
            GetMake();
            cout << endl << "DIRECTION:  ";
            GetDirection();
            cout << endl << "MODEL:      ";
            GetModel();
            cout << endl << "SPEED:      ";
            GetSpeed();
            cout << endl << "-----------------------------------------" << endl;
        }
        unsigned int GetColour() {
             switch (m_Colour) {
                case 1:
                    cout << "Red";
                    break;
                case 2:
                    cout << "Blue";
                    break;
                case 3:
                    cout << "Green";
                    break;
                case 4:
                    cout << "Yellow";
                    break;
                default:
                    cout << "Invalid";
                    return false;
            }
            return true;
        }
        void SetColour(unsigned int val = 1) {
            if ((val > 4) || (val < 1)) {
                cout << "Error assigning colour. Use numbers 1-4 only";
            } else {
                m_Colour = val;
            }
        }

        unsigned int GetMake() {
            switch (m_Make) {
                case 1:
                    cout << "Ford";
                    break;
                case 2:
                    cout << "Honda";
                    break;
                case 3:
                    cout << "Toyota";
                    break;
                case 4:
                    cout << "Pontiac";
                    break;
                default:
                    cout << "Invalid";
                    return false;
            }
            return true;

        }
        void SetMake(unsigned int val = 1) {
            if ((val > 4) || (val < 1)) {
                cout << "Error assigning Make. Use numbers 1-4 only";
            } else {
                 m_Make = val;
            }
        }
        unsigned int GetModel() {
            switch (m_Model) {
                case 1:
                    cout << "Truck";
                    break;
                case 2:
                    cout << "Car";
                    break;
                case 3:
                    cout << "Van";
                    break;
                case 4:
                    cout << "Super Bike";
                    break;
                default:
                    cout << "Invalid";
                    return false;
            }
            return true;
        }
        void SetModel(unsigned int val = 1) {
            if ((val > 4) || (val < 1)) {
                cout << "Error assigning Model. Use numbers 1-4 only";
            } else {
                 m_Model = val;
            }
        }
        unsigned int GetSpeed() {
            cout << m_Speed << "km/h";
            return true;
        }
        void SetSpeed(unsigned int val) {
            if (val > 200) {
                    cout << "Too fast (max speed: 200km/h)";
            } else {
                m_Speed = val;
            }
        }
        unsigned int GetDirection() {
            switch (m_Direction) {
                case 1:
                    cout << "North";
                    break;
                case 2:
                    cout << "East";
                    break;
                case 3:
                    cout << "South";
                    break;
                case 4:
                    cout << "West";
                    break;
                default:
                    cout << "Invalid";
                    return false;
            }
            return true;
        }
        void SetDirection(unsigned int val = 1) {
            m_Direction = val;
        }
        void ChangeDirection(int val = 1) {
            int tempDirection = m_Direction + val;
            if (tempDirection > 4) {
                tempDirection -= 4;
            }
            if (tempDirection < 1) {
                tempDirection += 4;
            }
            SetDirection(tempDirection);
        }
    protected:
    private:
        unsigned int m_Colour;
        unsigned int m_Make;
        unsigned int m_Model;
        unsigned int m_Speed;
        unsigned int m_Direction;
};

#endif // CAR_H

Answer 1

There are quite a few things you can improve. Here are some:

1. Don't using namespace in a header file. That's a bad idea. Refer to this SO discussion for details. Here you are better off qualifying your cout calls with std::.

2. Your class methods are too lengthy to be declared inline in the header file. You should move most of them, if no all, to the .cpp. Usually, only inline small methods like getters and setters that only set/return a variable.

3. car is never inherited from, so it should not have a virtual destructor. Virtual destructors are associated with inheritance. Until you have an actual need to inherit from car, make the destructor non-virtual. Also, since the destructor is a no-op, it could be omitted altogether, when not virtual. Reference: When should my destructor be virtual, C++ FAQ.

4. I'll second for the suggestion of using enums for the car properties. int convey no meaning, other than the fact that it is a whole number. Enums like Color, Maker and Model would be self-documenting and type safe (e.g. can't assign a Color to a Maker).

5. Methods that don't mutate member data, such as your Get*s should be const. This is often referred as const correctness. Example:

   unsigned int GetColour() const { /* ... */ }
                            ^^^^^
   

6. Normally std::cerr is used to log execution errors. std::cout is for normal program output. That said, you might also consider throwing exceptions when the inputs in your methods don't match the expected values. However, most of your errors like "Error assigning Model. Use numbers 1-4 only" would go away if you used strongly typed enums.

7. As commented by @immibis, having Get* methods print/log things is highly unusual. That seems like a violation of the single responsibility principle.

8. Don't add access level labels to your class if that section is empty. You have a protected: section but no protected data or methods. Remove that line.

9. The mix of lower case for types and PascalCase for methods is unusual. The usual would be also PascalCase for the types, reserving camelCase (first letter lowercase) for variables. Though I would personally suggest PascalCase for types only and camelCase for variables and functions/methods (or anything that can have its memory address taken, being more specific).

10. Lastly, probably the most relevant thing: Initializing and object with Get/Set methods is considered bad OOP design. One reason is that if you forget to set up a field, you might end up with a partly constructed object. Other disadvantage is that you can't declare a const instance. The correct way to initialize your Car would be using a parameterized constructor:

    Car::Car(Colour colour, Direction dir, Maker maker, Model model)
        : m_colour(colour)
        , m_direction(dir)
        , m_maker(maker)
        , m_model(model)
    { }
    

    Also worth noting the use of a constructor initialization list. That's the proper way of initing member data in a constructor, by calling the constructors of the member objects. It is basically just a comma separated list starting with a :.

11. One final note about your usage example: you seem to have this line just to halt the termination of the program until the user types something:

     int randomCrapIDontCareAbout;
     ...
     cin >> randomCrapIDontCareAbout;
    

    In that case, it would be more elegant to use std::cin.get().

Answer 2

First of all you need to decide on the naming convention. You use CamelCase for the class methods. In this case the name of the class should be capitalised, like this: class Car:. If you don't like the capitalised class names (like in stl) then use the underscore-delimited class methods, like void print_current_car(). Honestly I prefer the former. Check Google coding style as a reference

Now to the coding style. You have a few Set... methods that change the private members to certain values. I would use the enumerators for them. Also, your getter and setter return different types. I would not mix them, or I would use Print to the getter to signify that it prints the value:

class Car
{
    public:
        enum Colour{
             Red,
             Blue,
             Green,
             Yellow
        };
        Car();
        virtual ~Car();
        void SetColour(Colour clr){m_Colour = clr;}
        Colour GetColour(){return m_Colour;}
        void PrintColour(){
            switch (m_Colour) {
                case Red:
                    cout << "Red";
                    break;
                case Blue:
                    cout << "Blue";
                    break;
                case Green:
                    cout << "Green";
                    break;
                case Yellow:
                    cout << "Yellow";
                    break;
            }
        };

    private:
        Colour m_Colour;
};

Also, the m_ prefix seems very MFC way. I would drop it

And lastly, I would keep only the inline methods in the .h file. I would move all the methods to the cpp file. Why do you have there only the ctor and dtor?