5

Here is the work flow of a class of my program:

Server class instanciation -> Creating socket -> Binding socket to addr:port -> Listening -> Handling clients

Should I put all that stuff in the constructor or in separated private functions ?

5
  • 3
    FYI: This question is actually language agnostic... Good OOP is good regardless of language. You should consider changing the tags to reflect this. – Maybe_Factor Sep 5 '17 at 4:30
  • 1
    Possible duplicate of Should I extract specific functionality into a function and why? – gnat Sep 5 '17 at 4:53
  • @gnat: Your question doesn't include the special case of a constructor. – Nurrl Sep 5 '17 at 8:50
  • 4
    @Maybe_Factor It's not language agnostic, and constructors differ substantially between OOP languages. Some don't have a constructor. C++ is notably restricted in that all member fields are initialized before the constructor body runs, which limits how you can structure your code. This is a good and legitimate question as it is – amon Sep 5 '17 at 8:51
  • Handling clients should definitely not be in the constructor. As for the socket, you could create it first (bind and all, before creating Server), then inject it into Server via constructor. You're trying too hard to minimalize your code, you aren't accounting for things like testing or ease-of-use. If there's no socket, there's no Server. Thus, you should create the socket first, make sure that doesn't encounter any problems. Then, once the socket is prepared, create a Server and pass the socket via constructor. – Dioxin Sep 7 '17 at 18:45
9

As a rule, you should minimise the amount of business logic in constructors... So, depending on what your class's function/responsibility is, you may choose to include socket creation and binging the socket to an address in the constructor, but most likely listening and handling should be done in separate methods.

11
  • 1
    As addition to the above answer - I would put bind into constructor only if the port is set and cannot be modified - so when bind fails there is no possibility to use the object. I would move bind outside the constructor if where is possibility to try several ports till you find one that is free and report is somehow to the client that will connect to the server. – Artemy Vysotsky Sep 5 '17 at 3:33
  • 7
    A constructor is best used to acquire persistent dependencies. It is not a place to do work on those dependencies. – candied_orange Sep 5 '17 at 4:09
  • @ArtemyVysotsky: I like your point of view, thanks for the addition. – Nurrl Sep 5 '17 at 8:54
  • @CandiedOrange: So I should pass my critical parameters to the constructor and work on then separately ? – Nurrl Sep 5 '17 at 8:55
  • @CandiedOrange But if there is a set of steps that must always be done on those dependencies, in a fixed order, such as OP's example, I would insist that being part of the constructor. Doing so removes the need for a complex life cycle (e.g. you MUST call A, THEN B, THEN C, or else some horrible inconsistency error will come to bite your ass 20 lines of code later), and removes the caller-side complexity. – Alexander Sep 5 '17 at 12:10
1

The constructor should do only one thing and do it well: construct the object (here an instance of Server).

No, don't do all this in the constructor ! Here is why

If you add more things such as creating a socket, and binding it to a port address, you will face multiple problems. For example:

  • what if the port binding went wrong: should the constructor fail ? Or should it return the instance, but in an error state that could allow later recovery ?
  • what if there could be different alternatives to do the same thing, for example bind the server to an IPv4 port or bind the server to an IPv6 address ?

Furthermore, if the constructor initiates operations that do not end, such as listening and handling client requests, all your code will execute on an unfinished object (i.e. before the constructor completes).

Finally, some languages will not allow to effectively derive a new ServerXXX type, and override some of its methods. In C++ for example, your Server constructor would run on the Server base object of ServerXXX and will not be aware of the overridden ServerXXX methods. Only once the Server constructor would be finished would it hand over to the ServerXXX constructor

How to do it better ?

There are several alternatives. Not knowing all the details, I could imagine to use a (GoF) builder pattern:

  • An abstract builder would define the general interface for buidling your server, by building the different parts needed.
  • A concrete builder would implement this interface for a given Server class, and provide the glue to assemble the parts.
  • A "client" would create the appropriate concrete builder and inject it into the director
  • A director would invoke the methods of the contrete buider in the appropriate order
  • The client would invoke the concrete builder to get the final ready to use Server.

This kind of approach is not ideally suited to perform running business, i.e. listening and handling request. This is definitibely not part of the construction process. Furthermore, this could need multithreading, one thread doing the listening, and several other threads handling the requests for the connected clients.

For this part, I'd suggest to go for a template method pattern that implements the event loop for the server. The main code would then be something like:

ServerBuilder builder; 
ServerDirector director(builder); 
director.construct();                  // creates the server and its components
Server server = director.get_result(); // server ready to use 
server.mainloop();                     
3
  • Builder exists to work-around the lack of optional parameters, which OP doesn't have (and using builder methods for required dependencies isn't safe - chance of creating a partially constructed object). I'm kinda curious as to why you feel all these extra dependencies would help (Builder, Director) - seems like excess complexity for a simple problem, violating YAGNI – Dioxin Sep 7 '17 at 18:56
  • @VinceEmigh The goal of the GoF builder pattern is to decouple a generic building process from a specific type, the builder acting as an adapter for the construction process. It is often misunderstood, i suspect because it is often confused with Bloch's popular builder pattern. that adresses the problem of too many mandatory constructor parameters. I let to the OP judge if it's appropriate for his problem (i.e. Added complexity vs. Increased reusability). But regardless of this cherry on the cake, the fundamental answer of not doing all these operations in the constructor remains valid, no? – Christophe Sep 7 '17 at 20:23
  • Sorry, my statement was too problem-specific. You're right, it's not just for telescoping, and the answer is definitely valid. What I meant was there's no compile-time enforcement when using builder methods for required dependencies. Using it for required data makes code more fragile, as a developer isn't forced to perform a critical operation, which is why I was wondering: Why you'd prefer using builder there are no optional behaviors to instantiate Server (trying to learn from you, not down your post) – Dioxin Sep 7 '17 at 21:22
0

If your constructor is large enough that you're thinking hard about breaking it into several pieces, I'd tend to think about whether that might not indicate that your class (and its area of responsibility) is larger than it should be, so you might want to break the class into multiple pieces instead.

In the specific case of a socket server, I think there are really two (almost) completely separate parts. One just listens for incoming connections. It's pretty generic--almost any socket server does pretty much the same things: create a socket, bind it to an address, listen for incoming connections. Once that accepts a connection, it hands it off to some other class that has all the business logic for handling interactions with the client.

That second class is a session handler that just receives an open socket that's already been accepted, and communicates with the client via the socket. Harder to say exactly what happens here, because most of it depends on that interaction between the client and server.

When I've written code like this, neither class has had a constructor so large or complex that I saw much (any, really) reason to break it up into pieces. The listener basically does socket(), bind(), and listen(). Something like a dozen lines of code--and three fourths of that is error handling and such (though if you need to handle something like SSL/TLS, the complexity does grow somewhat).

0

I prefer constructors that return working instances of the given class. (Real-world analogy: if you order a car, you expect it to be functional in the sense that you can drive away with it, and that's what I expect from a constructor).

Of course, the definition of "working" is application-specific. So the question is, what do you expect from a Server?

  • Class instanciation: You surely need to do that, otherwise there's nothing you could call a Server.
  • Creating socket, Binding socket to addr:port, Listening: Is it a Server if it doesn't listen on any port?
  • Handling clients: Is it a Server if it doesn't handle client requests?

Typically, I'd answer all these questions with a YES, and have the constructor do all these things (not creating a 100-lines long constructor, but breaking the actions down into individual methods, of course). But after the Server has been constructed, it serves.

Maybe your answers are different, or maybe Server is meant to be an abstract base class - then the constructor should be different, of course.

0

Others answers cover most well. One thing not mentioned is how IDisposable(.NET) and AutoClosable and Closable(Java) interfaces work.

It is good practice in .NET to use the using statement and try-with-resources where possible.

If we put the logic in the constructor this would not be possible. So that is one more "con" for the pros and cons list.

-1

Assuming you have a class which, among other things, handles the initial binding of the port and setting up event handling as a must (aka absolutely must run before any other operation can be performed), one way you could do it is to create a public method init that must be called before any other. ...But that's ugly. More to the point, it requires that the caller take considerations that should be obscured by the overall class itself.

A better solution would be to lazily initialize the socket and event handling setup. Which is to say, you have an instance of Server with a private boolean member isInitialized initially set to false. Before any method is called, you perform this check and perform the necessary call to private method init, afterwards setting isInitialized to true. If you have few public methods, this is ideal and dirties your actual implementation very little. If you have many public methods, you should consider creating a second class which deals with the nitty gritty of the connection, which you use to perform the low-level actions, including establishing when the connection is formed (all of which would be obscured from the caller to Server).

While it's true that you're not likely going to create a Server instance without using it (so the secondary benefit of lazy loading doesn't really apply), it still provides you a means to remove potentially explosive code inside your constructor. The error, too, would be more thrown in a more intuitive spot, in a called method init inside your send method, which is likely a place which will be covered by try catch blocks anyway.

2
  • 3
    Introducing an extra uninitialized state should not be done lightly. For starters this requires messing with constness or mutable fields, and complicates the object life cycle thus increasing the risk of bugs. There is no particular reason why a throwing init() method would be better than a throwing constructor, unless construction has to be noexcept for some obscure reason. – amon Sep 5 '17 at 8:47
  • @amon If you're opening a socket, the class is at least conceptually stateful not stateless. Attempting to force a class to be stateless would be a mistake. Also the init method means you separate the creation of an instance with its logic (both in my example as a public class as well as being lazily called afterwards). – Neil Sep 5 '17 at 9:42
-2

I certainly prefer breaking apart big constructors, with a caveat:

I would only use static, preferably pure, methods for the job. This alleviates the need to worry about calling instance methods on an inconsistent instance.

For example, I might have something like:

class MyClass {
private:
    Socket socket;
public:
    MyClass(Address address, Port port) {
        this->socket = MyClass::createSocket();
        MyClass::bindSocket(this->socket, address, port);
        // etc
    }

    Socket createSocket() const {
        return Socket(); // hypothetical. Add socket construction logic here
    }

    void bindSocket(Socket socket, Address address, Port port) {
        socket.bind(address, port); //hypothetical
    }
};
5
  • 2
    I don't think you really know c++. Your code would fail compilation for many reasons. – BЈовић Sep 5 '17 at 6:32
  • @BЈовић There's a reason I said "something like", I didn't spend the time to run this through a compiler and make it into valid C++. Though it's been a while, so I probably should have. – Alexander Sep 5 '17 at 12:06
  • @BЈовић This question was tagged C++, so I did my best to illustrate my point in C++ for OP's convenience. The tag was since removed, as this really is a language agnostic question. I'm not sure where your issue is. – Alexander Sep 5 '17 at 12:07
  • My issue here is doing anything in the constructor besides remembering the address and port. At most validation logic could also go here. Neither creating the socket nor binding should happen just because this class was constructed. – candied_orange Sep 5 '17 at 17:05
  • BoundSocket bs = new MyClass( new SocketFactory().createSocket(), address, port) ).bindSocket() – candied_orange Sep 5 '17 at 17:31

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.