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:
While a minor annoyance, this seems further evidence to me that the private inheritance solution should be avoided even if it is temptingly compact.
1 Answer 1
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 typedef
s by using
s. It won't change much to your code, but it is easier to read, more flexible, and typedef
s 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.
-
\$\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 providebegin()/end()
at the container rather thanBoard
level for the reasons given in the question, and I figured if a client was accessing the container usingBoard::ports()
I might as well includeoperator[]
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– Null2018年02月01日 15:36:20 +00:00Commented Feb 1, 2018 at 15:36 -
\$\begingroup\$ Re: the
using
directive, how is it more flexible thantypedef
when it's creating a type definition? \$\endgroup\$Null– Null2018年02月01日 15:38:44 +00:00Commented Feb 1, 2018 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– Toby Speight2018年02月01日 15:52:56 +00:00Commented Feb 1, 2018 at 15:52
Board
) isn't a container in the standard library sense (vector
,map
, etc.). For example, it doesn't have avalue_type
and it doesn't really make sense for it to. Nonetheless, I saw your question and it's interesting -- I upvoted it. \$\endgroup\$