"Code Critique
In the following, I will go through the code and point out some thoughts along the way. Some of them are merely stylistic, but others can be considered bugs. The list of comments shouldn’t be considered exhaustive.
class SelectionStateController
{
protected:
typedef std::map
// a boolean selection state (true if selected)
typedef States::const_iterator CIt;
typedef States::iterator It;
// the function object to set up the list of displayed managers
struct Value
{
States::value_type operator()(const std::string &id) { return States::value_type(id,false); }
};
protected:
Container &clients; // a collection of clients to notify about selection state changes
Holding the client list as a reference can be a problematic choice. It makes it impossible for the controller to enforce invariants on the client list and to control list management. Admittedly, neither of those requirements were specified in the problem that was presented.
// returns the selection state of the manager 'id', true if selected
CIt cit=states.find(id);
#ifdef _DEBUG
check(cit,id);
#endif
C++ has a shortcut for this: assert(check(cit, id)). Spelling it out just adds visual clutter.
return cit->second;
}
// setters
void set_state(const std::string &id, bool new_state)
{
// sets the selection state of the manager 'id', notifies clients if the selection state is changed
It it=states.find(id);
#ifdef _DEBUG
check(it,id);
#endif
bool &state=it->second;
if (state!=new_state)
{
// update state and notify clients about the selection state change
state=new_state;
for (Container::iterator cont_it=clients.begin(); cont_it!=clients.end(); ++cont_it)
Standard C++ requires the keyword typename in front of Container::iterator as Container is a dependent type. MSVC doesn’t catch/enforce this – yet.
{
SelectionStateChangedFunc selection_state_changed(*cont_it);
selection_state_changed(id,new_state);
}
}
}
template
void set_displayed_managers(InIt first, InIt last)
{
// sets the list displayed managers in a range from the 'first' to the 'last'
// note: this method should be redesigned if selection states must be preserved upon changing the list of managers
states.clear();
std::transform(first,last,std::inserter(states,states.begin()),Value());
}
protected:
void check(CIt cit, const std::string &id) const throw(std::logic_error)
{
// checks if the manager 'id', which is pointed by the iterator 'cit', exists in the list of displayed managers
// throws the exception std::logic_error if it does not exist
if (cit==states.end())
throw std::logic_error("Manager "+id+" does not exist");
}
If I understand correctly, check is used as a way to check preconditions on function arguments. Essentially, it is an assertion; as such, it should never throw an exception. Assertions are debugging tools. Throwing an exception will unwind the stack, thus destroying valuable state information when debugging the issue.
};
class Client
{
public:
// function object that is invoked to notify about changing a manager's selection state
struct SelectionStateChanged
{
Client &client;
SelectionStateChanged(Client &client) : client(client) { }
Single argument constructors should be explicit unless an implicit conversion is really needed. In this case, the implicit conversion hides a subtle bug in the code (discussed below).
void operator()(const std::string &id, bool new_state)
{
std::cout
<< "Client " << client.id
<< " was notifyed about changing selection state, manager=" << id
<< ", new state=" << new_state << std::endl;
}
};
private:
friend struct SelectionStateChanged;
const int id;
Having id be a const member makes Client non-assignable, as assignment would require changing id in the assigned-to object. All standard containers required their value_type to be assignable.
public:
// constructor
Client(int id) : id(id) { }
// type conversion operator that constructs a notification funcation object
operator SelectionStateChanged() const { return SelectionStateChanged(*this); }
Implicit conversions are frowned upon in C++, as they almost invariably lead to problems in all but simple code bases. In this case, the conversion operator is never called as SelectionStateChanged’s conversion constructor is preferred over this function. If it were to be called, the program would crash after running out of stack space because it is recursive. Since the conversion operator is a const member function, *this is a const lvalue. It thus cannot be used as the argument to SelectionStateChanged’s conversion constructor that was defined requiring a non-const lvalue. What gets called is SelectionStateChanged’s implicitly defined copy constructor after calling Client::operator SelectionStateChanged() const on *this. This recursion leads to the crash.
};
int main()
{
// define a collection of clients
typedef std::list
Client is not assignable so this is not guaranteed to compile on a standard compliant std library. It will fail in future concepts-enabled code.
Clients clients;
std::back_inserter(clients)=Client(1);
std::back_inserter(clients)=Client(2);
The use of std::back_inserter here is odd. I can’t come up with a reason why std::list::push_back is not the better solution. Apart from that, it’s not std compliant. The standard requires output iterators to be dereferenced upon assignment. The fact that it works is an implementation artifact of the std library used."
