3
\$\begingroup\$

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<QImage*>& 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 << c;
    }

private:
    /* Private data members */

    QList<Component*> 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_received;
    QString m_type;
    QString m_maker;
    QString m_model;
    QString m_color;
    QString m_plate;

    QList<QImage*> m_images;
    QImage m_thumbnail;
};

class Car : public Vehicle
{
public:
    /* Public interface */

    //ctor
    Car();

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

    //dtor
    virtual ~Car();

private:
    /* Private data members */

    Body* m_body;
    Brake* m_brake;
    Direction* m_direction;
    Engine* m_engine;
};

I also have Moto and Bike classes publicly inheriting from Vehicle, each of them having their own components. Then, each component derives from Component and has several parts (deriving from Part, i.e. a Body has Wings, Capote, etc). Both Component and Part inherit from AbstractComponent.

First off, is this bad design? How can it be improved?

Secondly, I have a question about creating the specific vehicle objects. Should Car's constructor require the same parameters as Vehicle plus others? How about its components? Should I pass to the constructor pointers to already created Components, or should the Components be created in Car's constructor?

\$\endgroup\$
5
  • 2
    \$\begingroup\$ Irrelevant to your design, but a lot of your comments aren't useful. Unless you were required to, explicitly labelling your constructor/destructor and access specifiers is unnecessary; it just adds bulk. \$\endgroup\$ – Carcigenicate May 18 '15 at 15:41
  • \$\begingroup\$ Ownership of pointers seems a bit unclear here. Who is deleting all those components in Car? The car itself or Vehicle? \$\endgroup\$ – glampert May 18 '15 at 16:08
  • \$\begingroup\$ @Carcigenicate yes, you are indeed right \$\endgroup\$ – Tedy S. May 18 '15 at 16:38
  • \$\begingroup\$ @glampert pointers to components are deleted in Car. Vehicle keeps track of them only for generally iterating the components of a vehicle to see if any needs to be repaired \$\endgroup\$ – Tedy S. May 18 '15 at 16:40
  • \$\begingroup\$ I also just noticed that your vehicle constructor is massive. I don't know how you'd shrink it, but it's probably the biggest I've ever seen. \$\endgroup\$ – Carcigenicate May 18 '15 at 17:00
3
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.