Return to Revisions

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