39

I'm creating a server application in C++11 using Boost.Asio. I've created a class, Server, which takes care of accepting new connections. It's basically just:

void Server::Accept() {
  socket_.reset(new boost::asio::ip::tcp::socket(*io_service_));
  acceptor_.async_accept(*socket_,
                         boost::bind(&Server::HandleAccept, this, boost::asio::placeholders::error));
}

void Server::HandleAccept(const boost::system::error_code& error) {
  if (!error) {
    // TODO
  } else {
    TRACE_ERROR("Server::HandleAccept: Error!");
  }
  Accept();
}

I've found two ways (I'm sure there are more) to "fix" the TODO comment, i.e. to move the socket to wherever it should go. In my case I just want it back to the class instance that owns the Server instance (which then wraps it in a Connection class and inserts it to a list).

  1. Server has a parameter in its constructor: std::function<void(socket)> OnAccept which is called in HandleAccept.
  2. I create an abstract class, IServerHandler or whatever, which has one virtual method OnAccept. Server takes IServerHandler as parameter in its constructor and the class instance owning the server instance extends IServerHandler and constructs Server with *this as parameter.

What are the pros and cons of option 1 vs option 2? Are there any better options? I'm having the same problem in my Connection class (OnConnectionClosed). Also, depending on how I decide to design the system, it might need a OnPacketReceived and OnPacketSent callback.

simon
  • 2,042
  • 2
  • 20
  • 31
  • How are you managing lifetimes? I find `weak_ptr>` makes a good callback (the called owns the `shared_ptr`, and uninstalls it by `.reset()`). – Yakk - Adam Nevraumont Mar 12 '14 at 23:23
  • I prefer `1` using `std::function` callbacks stored as variables, that way they can be set later and checked before calling with `if ( onData ) onData( ptr, sz )` and so on – stefanB Mar 13 '14 at 00:33
  • 1
    One particular case where inheritance is preferable is when the object is movable; a lambda capturing the moved-from object then becomes invalid, so the move constructor/assignment needs to create a brand new lambda for the destination object. Inheritance would maintain the validity of the `this` pointer. – DanielKO Mar 13 '14 at 13:07
  • Check out this basis for a design design: http://ideone.com/2hC72x It's based on http://channel9.msdn.com/Events/GoingNative/2013/Inheritance-Is-The-Base-Class-of-Evil – Nicolas Louis Guillemot Mar 14 '14 at 09:22

4 Answers4

49

I strongly prefer the first way for several reasons:

  • Representing concepts/functionality via interfaces/class hierarchies makes the code base less generic, flexible, and then more difficult to mantain or scale in the future. That kind of design imposes a set of requirements on the type (the type implementing the required functionality) which makes it difficult to modify in the future, and most prone to fail when the system changes (Consider what happens when the base class is modified in this type of designs).

  • What you called the callback approach is just the classic example of duck typing. The server class only expects a callable thing which implements the required functionality, nothing more, nothing less. No "your type must be coupled to this hierarchy" condition is required, so the type which implements handling is completely free.

  • Also, as I said the server only expects a callable thing: It could be anything with the expected function signature. This gives the user more freedom when implementing a handler. Could be a global function, a bound member function, a functor, etc.

Take the standard library as an example:

  • Almost all standard library algorithms are based on iterator ranges. There is no iterator interface in C++. An iterator is just any type which implements the behaviour of an iterator (Being dereferenceable, comparable, etc). Iterator types are completely free, distinct, and decoupled (Not locked to a given class hierarchy).

  • Another example could be comparators: Whats a comparator? Is just anything with the signature of a boolean comparison function, something callable which takes two parameters and returns a boolean value saying if the two input values are equal (less than, bigger than, etc) from the point of view of a specific comparison criteria. There is no Comparable interface.

David Rodríguez - dribeas
  • 204,818
  • 23
  • 294
  • 489
Manu343726
  • 13,969
  • 4
  • 40
  • 75
  • +1 and I prefer to have these callbacks as instance variables, like `onData`, `onConnect`, `onDisconnect` it depends what is required, then they are easy to hook up, and before calling them you check `if ( onData ) onData( ptr, sz )` ... easy – stefanB Mar 13 '14 at 00:31
  • 10
    There are two sides to the coin, this answer focuses on the pros of the *callable* approach. On the other side, complex systems using just callbacks are much harder to reason about and maintain as the execution flow of the program is not clear from the code (something is called, go figure what). Each one of the approaches has its benefits and its place in complex systems. – David Rodríguez - dribeas Nov 26 '14 at 14:14
  • @DavidRodríguez-dribeas I was focused on the advantages of callables (being generic via duck typing the first one I think) vs interfaces. But I'm agree with you, reducing coupling is a good thing, but reducing it too much can make following the program flow harder. Thank you for posting that comment, and the edit fixing my misspellings – Manu343726 Nov 26 '14 at 20:47
1

What version of boost are you using? The best way IMHO is using coroutines. The code will.be eeasier to.follow. It will look like synchronous code but now I cannot give a comparison since I am writing from a mobile device.

Germán Diago
  • 7,473
  • 1
  • 36
  • 59
1

Just to mention that in many cases you do PREFER binding to a specific type.
Therefore in this case declaring that your class MUST have a IServerHandler helps you and other developers understand what interface they should implement in order to work with your class.
In future development when you add more functionality to IServerHandler you force your clients (i.e. derived classes) to keep up with your development.
This might be the desired behavior.

Ran Regev
  • 361
  • 3
  • 13
  • As you build up larger and larger systems, the dependency of each object on other interfaces is very important and it helps you keep the system tight, coherent and safer. Going back to the 'just pointer to a function' without the object encapsulation is sometimes just too much freedom. – Ran Regev Mar 18 '14 at 12:33
  • 1
    but you're ultimately not interested in the actual type of the client, just that it has something that abides to a defined signature that you can call. Right? – Johann Gerell Mar 18 '14 at 13:47
  • sometimes you prefer to have a group of functions that have something in common. For example, it does not have a meaning to `send()` or `receive()` if you didn't `connect()`. therefore it would be better to have a TYPE that answer/implement all the relevant functions. There is no meaning to just call `send()` without the correct _context_ i.e. a fully built object. The idea of encapsulating functionality, as well as data, together has a great benefits that people tend to forget. – Ran Regev Mar 19 '14 at 11:49
  • 1
    In that case just write a policy which its parametrized with the send, conect, and receive policies. Far more flexible – Manu343726 Mar 19 '14 at 17:30
  • Object Oriented is a good thing. All I'm trying to say is that sometimes just an 'old school' class with interface functions and the related data - this is what is needed. The fact that one can wrap everything with std::function<> does not mean that this is the good thing to do. You really want to minimize the mistakes your clients may do when over-freedom is present. – Ran Regev Apr 07 '14 at 09:45
1

It all boils down to your intentions.

On one hand, If you want to expect that an functionality belongs to a specific type, then it should be implemented in terms of it's hierarchy, such as a virtual function, or a member pointer, etc. Limitation in this sense is good because it helps to makes your code easy to use correctly, and difficult to use incorrectly.

On the other hand, if you just want some abstract "go here and do this" functionality without having to bother with any burden of it being tightly coupled to a specific base class, then clearly something else will be more appropriate like a pointer to a free function, or an std::function, etc.

It's all about which is more fitting to the specific design of any specific part of your software.

Brandon
  • 483
  • 3
  • 12