Representing vehicles and their components

I am working on a project that needs to represent different kinds of vehicles, components of these vehicles and actual parts of the components.

So far I have this code:

```
class Vehicle
{
public:
/ Public interface /

//ctor
Vehicle(int id, int year, int mileage, int weight, int length, int width, int height, int wheelbase,
const QString& date, const QString& type, const QString& maker, const QString& model, const QString& color, const QString& plate);

//copy/move
Vehicle(const Vehicle&) = delete;
Vehicle(Vehicle&&) = delete;
Vehicle& operator=(const Vehicle&) = delete;
Vehicle& operator=(Vehicle&&) = delete;

//dtor
virtual ~Vehicle();

//getters
inline int getId() const { return m_id; }
inline int getYear() const { return m_year; }
inline int getMileage() const { return m_mileage; }
inline int getWeight() const { return m_weight; }
inline int getLength() const { return m_length; }
inline int getWidth() const { return m_width; }
inline int getHeight() const { return m_height; }
inline int getWheelbase() const { return m_wheelbase; }

inline const QString& getDateReceived() const { return m_date_received; }
inline const QString& getType() const { return m_type; }
inline const QString& getMaker() const { return m_maker; }
inline const QString& getModel() const { return m_model; }
inline const QString& getColor() const { return m_color; }
inline const QString& getPlate() const { return m_plate; }

inline const QList& getImages() const { return m_images; }
inline const QImage& getThumbnail() const { return m_thumbnail; }

protected:
/ Protected methods /

//adds another component to this vehicle
inline void addComponent(Component* c)
{
m_components m_components;

int m_id;
int m_year;
int m_mileage;
int m_weight;
int m_length;
int m_width;
int m_height;
int m_wheelbase;

QString m_date_r

The architecture seems fine to me the way it is. It's nice that you have used a little bit of componentization instead of doing some absurd inheritance chain in Car. Maybe someone else will have more comments on the design, but if you are not having problems with it, my opinion is to leave it as it stands.

A few other details I would like to comment on:

-
There doesn't seem to be any method that changes a Vehicle after an instance is constructed. This is a good design. You should favor immutability whenever practical. Since this is the case, you should further enforce that the vehicles are not mutated by making its member variables const. Then you only allow setting them once in the constructor. E.g.:

const int m_id;
const int m_year;
const int m_mileage;
const int m_weight;
const int m_length;
const int m_width;
const int m_height;
const int m_wheelbase;
...

You probably won't be able to make the QLists const though. But that's not a big deal. Mark with const what you can.

-
Vehicle's constructor has a lot of parameters. It might be confusing to call it, since many parameters have the same type. One way to consolidate the large number of parameters would be having a sort of VehicleDesc helper structure grouping all the parameters. But that might or might not be a better solution, it depends. It also doesn't simplify setting up a Vehicle. Another possibility might be constructing from a stream or file (AKA serialize/de-serialize).

-
When you already delete the copy constructor and assignment operator you don't have to also delete the move ones. Remember that move operator and constructor are not added by default to all classes by the compiler, unlike the copy/assign ones. Quoting this very good StackOverflow answer:


Even if you want your class to not be copyable nor movable, it is
better to just delete the copy members and leave the move members
undeclared (meaning they won't exist). If you're ever reviewing code
(including your own), and see deleted move members, they are almost
certainly incorrect, or at the very best superfluous and confusing.

-
You don't need to add the inline keyword when declaring a member method directly inside the class body in the header file. Such methods are already implicitly (to the compiler) and clearly (to the reader) inline.

-
Seconding for a comment in your post, don't write self evident comments like //ctor or //copy/move. That's just visual pollution, plus, if you shuffle class sections around and forget to move the comments with it, it will create discrepancies.

-
About your question of Car's constructor replicating the parameters of Vehicle, if you have access to C++11, the you'll probably want to use constructor inheritance. Otherwise, I'm afraid you will have to replicate the constructor's parameter list and just forward the data to Vehicle.

-
Finally, the ownership of pointers in your code worries me a little. Car has raw pointers to its components, which I assume are also replicated in the components list of Vehicle. There is a certain potencial for memory bugs there. You are probably very familiar with the code and knows exactly when things get allocated and deleted, but don't expect the next guy you pass this project on to be that familiar with it (or even yourself might forget about it in a few months time!). People maintaining your code are much more likely to introduce bugs than you are. For this reason, mainly, I would advocate for the use of smart pointers. Car should probably hold shared_ptrs to its components, so that Vehicle can store weak_ptrs in its component list.

StackExchange Code Review Q#91067, answer score: 3