First OOP in C++ - Car example

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 
#include "car.h"

using namespace std;
int main()
{
    car MyCar;
    int randomCrapIDontCareAbout;
    cout > 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 

using namespace std;

class car
{
    public:
        car();
        virtual ~car();
        void PrintCurrentCar() {
            cout  4) || (val  4) || (val  4) || (val  200) {
                    cout  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

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

-
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::.

-
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.

-
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.

-
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).

-
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 { /* ... */ }
                         ^^^^^

-
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.

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

-
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.

-
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).

-
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 :.

-
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().

StackExchange Code Review Q#86341, answer score: 16