Mir

Code review comment for lp:~robert-ancell/mir/frontend-class-renaming

Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

> There is a possible argument that "Protobuf" prefixes are appropriate, should anyone want to add support for other protocols later

It's also about descriptive naming. ProtobufSocketCommunicator is a communicator that uses protobuf over sockets, all the important information is in the name.

> The only part that I don't like is that we're getting rid of MessageProcessor's interface class, will make it harder to mock.

> Dependencies should always be on interfaces, not concrete classes - so I think it wrong to conflate MessageProcessor and ProtobufMessageProcessor.

The fact that we can make MessageProcessor concrete without any consequences, tells me that we are missing some tests where the interface is used (in SocketSession, e.g. message_processed_when_new_message_arrives), not that we don't need to have it as an interface.

> > We've a rule in the codebase that we don't have null dependencies (and use null objects instead).
> It's not a rule; it's a convention. And any convention which results in trivial un(der)used classes should probably be changed, or at least bent.

However you call it, the point is that there are reasons for it, we didn't just flip a coin and went with NullObjects. If we turn this around: "any convention which results in the circulation of null smart pointers should probably be changed, or at least bent." :) As with any engineering decision, there are trade-offs involved, and the decision was that NullObject trade-offs are worth it.

review: Needs Fixing

« Back to merge proposal