5
\$\begingroup\$

I frequently design classes which model an object that has multiple submodules of the same type. For example, I have a class for a Printed Circuit Board (PCB) which has multiple subcircuits (I will call them "ports"). The class for the PCB (Board) uses a container (e.g. std::vector) as a member variable to store the subcircuits. Client code needs to be able to iterate over the ports to query the state of each port, control the port, etc, so I need to provide iterators for the ports.

In general the board may have multiple subcircuits of different types (and therefore multiple member containers that need to be iterated over). For example, the Board class might have a std::vector<Port> for the ports, a std::vector<double> for a data bus of voltage signals, etc. Thus I cannot simply provide begin() and end() member functions -- I would need to call them something like begin_ports() and end_ports() to differentiate the container of Port objects from other member containers. Such non-standard names would prevent me from using range-based for loops (which depend on functions named begin() and end()).

I searched around on Software Engineering SE and Stack Overflow to see if there was a standard solution for this, and I found a couple of interesting suggestions to create a container class that privately inherits from the appropriate standard container (e.g. here and here) and makes the accessor functions like begin()/end() public. However, this seems to go against advice to favor composition over inheritance (e.g. here and here) when composition is possible.

Below is a solution for a Board class that has multiple Port objects stored in a container called Ports that uses composition instead of inheritance. Board and Port are simplified from the actual code, and for this review I'd like to focus mainly on the Ports container class and the related accessor functions for it in Board. (Note that the first port is numbered 1, not 0, since that's how the ports are numbered on the physical board's schematic.)

Board.h

#ifndef BOARD_H
#define BOARD_H

#include <vector>

class Board {
public:
    class Port {
    private:
        bool p_enabled;

        double p_vout;
    public:
        Port();

        auto is_enabled() const {
            return p_enabled;
        }

        bool enable();

        bool disable();

        auto v_out() const {
            return p_vout;
        }

        bool v_out(double v);

        bool reset();
    }; // end class Port

    class Ports {
        // Every Ports container is owned by a Board so Board has access to its Ports
        // container's private members and functions.
        friend class Board;

        typedef std::vector<Port> container_type;

        container_type ports_;

        // Default-constructs a specified number of Board::Ports.
        // The constructor is private so only a Board can construct a Ports container.
        Ports(container_type::size_type count) : ports_(container_type(count)) {}

        // Deleted copy constructor.
        // An instance of the class cannot be copied from a reference to it.
        // This means the return value of Board::ports() can only be assigned to a reference.
        Ports(const Ports& other) = delete;
    public:
        typedef container_type::size_type size_type;
        typedef container_type::iterator iterator;
        typedef container_type::const_iterator const_iterator;

        const Port& operator[](size_type index) const;
        Port& operator[](size_type index);

        const Port& at(size_type index) const;
        Port& at(size_type index);

        iterator begin();
        const_iterator begin() const;
        const_iterator cbegin() const;

        iterator end();
        const_iterator end() const;
        const_iterator cend() const;
    };
private:
    Ports m_ports;

    double vsupply;
public:
    typedef Ports::size_type port_size_type;
    typedef Ports::iterator port_iterator;
    typedef Ports::const_iterator const_port_iterator;

    Board(double vsupply, port_size_type num_ports);

    const Ports& ports() const {
        return m_ports;
    }

    Ports& ports() {
        return m_ports;
    }

    const Port& port(port_size_type index) const {
        return m_ports[index];
    }

    Port& port(port_size_type index) {
        return m_ports[index];
    }

    const Port& port_at(port_size_type index) const {
        return m_ports.at(index);
    }

    Port& port_at(port_size_type index) {
        return m_ports.at(index);
    }

    double Vsupply() const {
        return vsupply;
    }

    bool Vsupply(double v_pwr);

    /** Resets every Port to its default settings.
    Returns true if any settings were changed on any Port, false otherwise. */
    bool reset();
}; // end class Board

#endif

Board.cpp

#include <stdexcept> // std::invalid_argument

#include "board.h"

Board::Board(double vsupply, port_size_type num_ports) : vsupply(vsupply), m_ports(num_ports) {
    if (vsupply < 0) throw std::invalid_argument("Vsupply < 0V is not allowed");
}

Board::Port::Port() :
    p_enabled(false),
    p_vout(0) {}

bool Board::Port::enable() {
    bool update = (p_enabled == false);
    p_enabled = true;
    return update;
}

bool Board::Port::disable() {
    bool update = (p_enabled == true);
    p_enabled = false;
    return update;
}

bool Board::Port::v_out(double v) {
    bool update = (p_vout != v);
    p_vout = v;
    return update;
}

bool Board::Port::reset() {
    bool update = false;

    update |= disable();
    update |= v_out(0);

    return update;
}

const Board::Port& Board::Ports::operator[](size_type index) const {
    return ports_[index - 1];
}

Board::Port& Board::Ports::operator[](size_type index) {
    return ports_[index - 1];
}

const Board::Port& Board::Ports::at(size_type index) const {
    return ports_.at(index - 1);
}

Board::Port& Board::Ports::at(size_type index) {
    return ports_.at(index - 1);
}

Board::Ports::iterator Board::Ports::begin() {
    return ports_.begin();
}

Board::Ports::const_iterator Board::Ports::begin() const {
    return ports_.begin();
}

Board::Ports::const_iterator Board::Ports::cbegin() const {
    return ports_.cbegin();
}

Board::Ports::iterator Board::Ports::end() {
    return ports_.end();
}

Board::Ports::const_iterator Board::Ports::end() const {
    return ports_.end();
}

Board::Ports::const_iterator Board::Ports::cend() const {
    return ports_.cend();
}

bool Board::Vsupply(double v_supply) {
    if (v_supply < 0) return false;

    vsupply = v_supply;
    return true;
}

bool Board::reset() {
    bool update = false;

    for (auto port : m_ports) {
        update |= port.reset();
    }

    return update;
}

Client code can obtain a reference to the Ports member for iterating over it and/or to call its operator[] and at member functions. Client code can also access a Port directly from Board using Board::port() or Board::port_at().

Here's a small demo program to show it in use with both mutable and const iterators, and accessing a specified Port:

#include <iostream>
#include <fstream>
#include <algorithm>

#include "board.h"

int main() {
    std::ofstream ofs("demo.txt");
    ofs << std::boolalpha;

    Board b(30, 8);
    const Board& bref = b;

    auto& ports = b.ports();
    auto& cports = bref.ports();

    double v = 0;
    int port_number = 1;

    ofs << "Vsupply = " << bref.Vsupply() << "V\n";
    b.Vsupply(25);
    ofs << "Vsupply = " << b.Vsupply() << "V\n\n";

    for (auto& port : b.ports()) {
        port.enable();
        port.v_out(v);

        v += 2.5;
        port_number++;
    }

    ports[5].reset();

    for (auto& port : cports) {
        ofs << "Vout = " << port.v_out() << "V\n";
        ofs << "enabled?: " << port.is_enabled() << '\n';

        ofs << '\n';
    }

    ofs << "All ports are enabled?: "
        << std::all_of(ports.cbegin(), ports.cend(), [](const auto& p) { return p.is_enabled(); })
        << '\n';

    return 0;
}

The demo program outputs:

Vsupply = 30V
Vsupply = 25V

Vout = 0V
enabled?: true

Vout = 2.5V
enabled?: true

Vout = 5V
enabled?: true

Vout = 7.5V
enabled?: true

Vout = 0V
enabled?: false

Vout = 12.5V
enabled?: true

Vout = 15V
enabled?: true

Vout = 17.5V
enabled?: true

All ports are enabled?: false

I like the way this turned out despite the fact that there's some extra typing compared to the private inheritance solution:1

class Ports : private std::vector<Port> {
    friend class Board;
    public:
        Ports(std::vector<Port>::size_type count) : std::vector<Port>(count) {}

        using std::vector<Port>::begin;
        using std::vector<Port>::cbegin;
        using std::vector<Port>::end;
        using std::vector<Port>::cend;
        using std::vector<Port>::operator[];
};

Can my solution be improved? Are there any C++11 or C++14 features I forgot to use that would improve it (I'm compiling with Visual Studio 2017)? Is the name Ports for the Port container a good choice or is it too similar to the objects it contains?

Also, I've made the Ports constructor private and deleted the copy constructor to try to make sure only a Board can construct a Ports and nothing can copy one -- are there any other ways to construct or copy a Ports that I've missed? Is it a good idea to prevent construction/copies or is it unnecessarily restrictive?


1 When I played with the private inheritance solution in the demo program's main() I discovered that Intellisense shows all of the underlying container's member functions despite the fact that most of them are inaccessible:

enter image description here

While a minor annoyance, this seems further evidence to me that the private inheritance solution should be avoided even if it is temptingly compact.

\$\endgroup\$
  • \$\begingroup\$ The standard advice is not to subclass standard containers, as they don't have virtual destructors. That's clearly not a problem if the inheritance is private; I'm not sure whether there are other reasons that may still apply (other than the one you've pointed out with your editor). \$\endgroup\$ – Toby Speight Feb 1 '18 at 15:56
  • \$\begingroup\$ I think you can highly generalize this problem. You really want an iterator that will iterate across a container of containers. That should be easy to write standalone without reference to your specific case. \$\endgroup\$ – Martin York Feb 1 '18 at 18:58
  • \$\begingroup\$ Does this help in any way? codereview.stackexchange.com/q/186544/507 \$\endgroup\$ – Martin York Feb 1 '18 at 20:01
  • \$\begingroup\$ @MartinYork I think this is a general problem seeing as I've run into it several times and my research on Software Engineering SE indicates others have as well. However, my problem is not quite as general as iterating a container of containers -- the "container" here (Board) isn't a container in the standard library sense (vector, map, etc.). For example, it doesn't have a value_type and it doesn't really make sense for it to. Nonetheless, I saw your question and it's interesting -- I upvoted it. \$\endgroup\$ – Null Feb 1 '18 at 23:01
3
\$\begingroup\$

I agree with the general direction of this code, but I think it could be made simpler.

First some comments about the C++11/14 features you could have used:

Data initialization: since C++11 (at least if I remember correctly) you can specify default values for non-static data members inside the class. For instance:

class Port {
private:
    bool p_enabled = false;
    double p_vout = 0;
public:
    Port();
// ...
};

It is a lot more readable than having, as you do, the members' definition in the .cpp file. The other possibility is to have an inline constructor with a member initializer list, which was already available with C++98.

using directive: you can replace typedefs by usings. It won't change much to your code, but it is easier to read, more flexible, and typedefs should progressively disappear anyway.

Now, onto simplification: I believe that you have to make a choice between exposing the whole ports container and exposing only some ways to access its content. There's absolutely no upside in offering begin, end, [] and so on if the client already has access to the vector inside, since they can call those function themselves with e.g. class.container().begin(). And the downside is clear since you have to maintain many transparent functions, over multiple layers.

So, either you want to hide some implementation details, or want to control how class members are accessed and/or modified, and then offering direct access to the inner containers isn't rational, or you don't mind and then providing numerous ways to access the same piece of data is just too much code.

\$\endgroup\$
  • \$\begingroup\$ You make a fair point about simplifying the ways to access the container's content. I thought Board::port() was worth including because I could see clients often accessing a specific port directly. I have to provide begin()/end() at the container rather than Board level for the reasons given in the question, and I figured if a client was accessing the container using Board::ports() I might as well include operator[] at the container level as well. The maintenance downside seemed minimal since these accessors are pretty trivial. It's something I'll consider, though. Thanks and +1. \$\endgroup\$ – Null Feb 1 '18 at 15:36
  • \$\begingroup\$ Re: the using directive, how is it more flexible than typedef when it's creating a type definition? \$\endgroup\$ – Null Feb 1 '18 at 15:38
  • \$\begingroup\$ You do remember correctly - C++11 introduced the syntax to specify initializers with the members (rather than requiring them on every constructor). \$\endgroup\$ – Toby Speight Feb 1 '18 at 15:52

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.