Don't
using namespace
in a header file. That's a bad idea. Refer to this SO discussionto this SO discussion for details. Here you are better off qualifying yourcout
calls withstd::
.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 avirtual
destructor. Virtual destructors are associated with inheritance. Until you have an actual need to inherit fromcar
, 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
enum
s for the car properties.int
convey no meaning, other than the fact that it is a whole number. Enums likeColor
,Maker
andModel
would be self-documenting and type safe (e.g. can't assign aColor
to aMaker
).Methods that don't mutate member data, such as your
Get*
s should beconst
. 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 alsoPascalCase
for the types, reservingcamelCase
(first letter lowercase) for variables. Though I would personally suggestPascalCase
for types only andcamelCase
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 yourCar
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) { }