Search code examples
c++refactoringc++17code-duplication

What can I do to remove or minimize this code duplication that will provide the same functionality and behavior?


I have a set of derived classes and I'm using a function to update their internal ID values automatically upon class instantiation or creation. Originally I had this function in their base or superclass and it was working giving me correct results. However, as a design decision, I'm in the process of changing this behavior.

In the first version, every time a Component object was being constructed, the appended numerical value was being incremented even when the sub-types were different. I don't want this behavior. I want each subclass to start at [0] and count up...

I moved the function that performed this calculation appending the new value to the member string id from the base class to the derived classes. However, this is now creating code duplication, and with this current design, every future subclass would have to implement this function...

I want the same functionality and behavior that I currently have, but without the code duplication!


This is my current output which is the behavior I'm looking for with the displaying of the IDs:

Successfully connected Wire_001 to Wire_005
         from Wire_001 to Bus_000
Successfully connected Wire_002 to Wire_006
         from Wire_002 to Bus_000
Successfully connected Wire_003 to Wire_007
         from Wire_003 to Bus_000
Successfully connected Wire_000 to Wire_005
         from Wire_000 to Bus_000
Component Wire_000 already exists in Wire_004!
         from Wire_000 to Bus_000

Successfully connected Wire_001 to Wire_000
Successfully connected Wire_002 to Wire_001
Successfully connected Wire_003 to Wire_002
Successfully connected Wire_000 to Wire_003

Successfully connected Wire_004 to Wire_008
         from Wire_004 to Bus_001
Successfully connected Wire_005 to Wire_009
         from Wire_005 to Bus_001
Successfully connected Wire_006 to Wire_010
         from Wire_006 to Bus_001
Successfully connected Wire_007 to Wire_011
         from Wire_007 to Bus_001

These are my connected components:
        Wire_004 has Wire_000

        Wire_005 has Wire_001 Wire_000

        Wire_006 has Wire_002

        Wire_007 has Wire_003

Here's my current source code:

main.cpp

#include <exception>

#include "Bus.h"

int main() {
    try {
        Wire w1, w2, w3, w4;
        std::cout << w1.id() << " " << w2.id() << " " << w3.id() << " "<< w4.id() << "\n\n";

        Bus<4> b1;

        b1.connect(&w1, 0);
        b1.connect(&w2, 1);
        b1.connect(&w3, 2);
        b1.connect(&w4, 3);

        b1.connect(&w1, 1);  // Valid Connection: same wire to multiple bus wires
        b1.connect(&w1, 0);  // Invalid Connection: trying to connect a same wire to the same bus wire multiple times...
        std::cout << "\n";

        Bus<4> b2;
        w1.connect(&w2);
        w2.connect(&w3);
        w3.connect(&w4);
        w4.connect(&w1);
        std::cout << "\n";

        b2.connect(&b1.wire(0), 0);
        b2.connect(&b1.wire(1), 1);
        b2.connect(&b1.wire(2), 2);
        b2.connect(&b1.wire(3), 3);

        std::cout << "\nThese are my connected components:\n";
        for( size_t i = 0; i < b2.size(); i++ ) {
            for (auto& component : b2.myConnections(i)) {
                std::cout << "\t" << component->id() << " has ";
                auto connections = component->myConnections();
                for (auto& c : connections) {
                    std::cout << c->id() << " ";
                }
                std::cout << '\n';
            }
            std::cout << '\n';
        }
                     
    } catch (const std::exception& e) {
        std::cerr << e.what() << std::endl;
        return EXIT_FAILURE;
    }
    return EXIT_SUCCESS;
}

Component.h

#pragma once

#include <assert.h>   
#include <memory>    
#include <array>
#include <list>    
#include <iomanip>
#include <iostream>    
#include <string>
#include <sstream>    

class Component {
protected:
    std::string id_ = "";
    std::list<std::shared_ptr<Component>> components_;

    explicit Component(const std::string& id) : id_{ id } {}

public:
    virtual ~Component() {}

    std::string& id() { return id_; }

    void connect(Component* other) {
        for (auto& l : components_) {
            if (other->id_ == l->id()) {
                std::cout << "Component " << other->id_ << " already exists in " << id_ << "!\n";
                return;
            }
        }

        components_.push_back( std::make_shared<Component>( *other ) );
        std::cout << "Successfully connected " << other->id() << " to " << id_ << "\n";
    }

    virtual std::list<std::shared_ptr<Component>> myConnections() { return components_; }    
    virtual void propagate() {};    
};

Wire.h

#pragma once

#include "Component.h"

class Wire : public Component {
private:

public:
    explicit Wire(const std::string& name = "Wire") : Component(name) {
        updateId();
    };
       
    virtual ~Wire() {}

    virtual void propagate() override {
        return;
    }

private:
    void updateId() {
        static int i = 0;
        std::stringstream strValue;
        strValue << std::setw(3) << std::setfill('0') << std::to_string(i++);
        id_.append("_" + strValue.str());
    }
};

Bus.h

#pragma once

#include "Wire.h"

template<size_t BusSize>
class Bus : public Component {
private:
    std::array<std::shared_ptr<Wire>, BusSize> bus_interface_;

public:
    explicit Bus(const std::string& name = "Bus") : Component(name) {
        updateId();
        createBus();
    }

    virtual ~Bus() {}

    virtual void propagate() override {
        return;
    }

    void connect(Component* component, size_t index) {
        assert(index < BusSize);
        assert(component != nullptr);
        bus_interface_[index]->connect(component);
        std::cout << "\t from " << component->id() << " to " << id_ << "\n";
    }

    Wire& wire(size_t index) {
        assert(index < BusSize);
        return *bus_interface_[index].get();
    }

    std::array<std::shared_ptr<Wire>, BusSize> bus() { return bus_interface_; }

    std::list<std::shared_ptr<Component>> myConnections(size_t index) { 
        assert(index < BusSize);
        return bus_interface_[index]->myConnections();
    }

    size_t size() const { return BusSize; }

private:
    void updateId() {
        static int i = 0;
        std::stringstream strValue;
        strValue << std::setw(3) << std::setfill('0') << std::to_string(i++);
        id_.append("_" + strValue.str());
    }

    void createBus() {
        size_t i = 0;
        for (auto& w : bus_interface_) {
            w = std::shared_ptr<Wire>(new Wire);
        }
    }
};

As you can see from the two subclasses, their member function ::updateId():

void updateId() {
    static int i = 0;
    std::stringstream strValue;
    strValue << std::setw(3) << std::setfill('0') << std::to_string(i++);
    id_.append("_" + strValue.str());
}

is the code duplication that I'm concerned with. What can I do to remove the dependency of this code duplication while keeping the same functionality and behavior?


Solution

  • You can make use of CRTP. Add a templated second base class to hold the ID handling code.

    template <class T>
    class ComponentID {
    public:
        ComponentID(std::string &id_) {
            // Same as updateID above
        }
    };
    
    class Wire: public Component, ComponentID<Wire> {
    public:
        Wire(const std::string& name = "Wire") : Component(name), ComponentID(id_) {
        }
        // ...
    };
    
    template<size_t BusSize>
    class Bus : public Component, ComponentID<Bus<BusSize>> {
        public:
            explicit Bus(const std::string& name = "Bus") : Component(name), ComponentID(id_) {
                createBus();
            }
            // ...
    };
    

    We're passing in the id_ field from the Component base class to the ComponentID constructor so that it can be updated without having to change the access to id.