Return to Answer

I did not review the serverSock but most comments that apply to the client socket also apply to the server socket.

##Self Plug/ Further reading I started writing a series of Blogs about Socket programming. Still not complete. But as part of that series you will find a socket wrapper class.

This is particularly bad here because it is in a header file. But you should not even use it in a source file.

Doing this makes your code much more susceptible to bugs:

read: Why is “using namespace std” considered bad practice?

But putting it in a header file will also pollute other people's code. If I include your header file in my code it could potentially break my code in subtle ways. Things like that get you band from projects.

Its a bad habit and you should break it as soon as possible. The reason the "Standard" namespace is called std:: is so that it is not difficult to use it as a prefix in-front of Types/Objects.

An object should be created correctly or not created at all. Never use the constructor to try and set up a connection then force the user to check to the state with an additional call. This will result in your code having bugs as users of your code will forget to check the state.

Don't do it. If the constructor fails then throw an exception.

###Error Codes.

If something goes wrong that you can't fix locally then throw an exception. If the user of the code does not catch the exception then it will correctly terminate the application. You can then find this in testing and add the appropriate code to fix the problem (or ignore it as appropriate).

As it stands your object is copyable but not movable. Remember the compiler generates the copy constructor and copy assignment operator for you by default.

I would think the opposite should be true. Especially since the copy semantics do not follow the rule of three (default copy semantics do not cope with owned resources correctly and a file handle is an owned resource).

{
    clientSock  sock1("google.com", 80);
    clientSock  sock2(sock1);
}
// Destructor calls close on sock2
// Destructor calls close on sock1 (same as sock1)
// Probably not devastating but definitely untidy.

But why try and connect three times? Is there some reason you think things will change. If they do change what does that mean about the error that happened the first two times? This code is too low level to make decisions like this. If there is an error here you should be getting a bit of code that has much more context to make the decision to try and reconnect again.

###Pass parameters by const reference (when they are non mutable objects).

This code is way to low level to be printing error messages.

A whole bunch of those error codes should never happen (assuming your socket code works). So there are error messages that should never happen here (caused by bugs in the code (bad file descriptor)) and there are error messages that are transient (network cable was unplugged). You should distinguish between the two types in your errors.

Which this code seems to do. But this is supposed to be low level socket code. So it should not be handling protocols. This code should be in a derived class for the specific protocol that it understands.

I did not review the serverSock but most comments that apply to the clientSock also apply to the server socket.

##Self Plug  / Further reading I started writing a series of blogs about Socket programming; Still not complete. But as part of that series you will find a simple socket wrapper class.

This is particularly bad here because it is in a header file. You should not use it in a source file either (bad habit).

Doing so makes your code much more susceptible to bugs, read: Why is “using namespace std” considered bad practice?

Putting it in a header file is worse because it can pollute other people's code (any code that includes your header file). If I include your header file in my code it could potentially break my code in subtle ways. Things like that get you band from projects.

Its a bad habit and you should break it as soon as possible. 

The reason the "Standard" namespace is called std:: is so that it is short and therefore not "much" of a burden to use as a prefix in-front of Types/Objects.

An object should be created correctly or not created at all. Never use the constructor to try and initialize an object that fails; then force the user to check to the state with an additional call. This will result in your users code having bugs as users of your code will forget to check the state.

If the constructor fails then throw an exception.

###Error Codes

If something goes wrong that you can't fix locally then throw an exception. If the user of the code does not catch the exception then it will correctly terminate the application (any other action is a bug without knowing the context). You can then find this in testing and add the appropriate code to fix the problem (or ignore it as appropriate).

The compiler generates the copy constructor and copy assignment operator for you by default. So the following are not your only constructors.

Compiler generated:

    clientSock(clientSock const&);
    clientSock& operator=(clientSock const&);

This allows:

{
    clientSock  sock1("google.com", 80);
    clientSock  sock2(sock1);
}
// Destructor calls close on sock2
// Destructor calls close on sock1 (same as sock1)
// Probably not devastating but definitely untidy.

Personally I would disable the copy semantics of the object.

Conversely I would enable the move semantics of a socket object. So that it can be passed into functions etc.

    clientSock(clientSock&&);             // Notice the double &&
    clientSock& operator=(clientSock&&);

But why try and connect three times? Is there some reason you think things will change? If they do change what does that mean about the error that happened the first two times? This code is too low level to make decisions like this. If there is an error here you should be getting a bit of code that has much more context to make the decision to try and reconnect again.

###Pass parameters by const reference (when they are non-mutable objects).

This code is way too low level to be printing error messages.

A whole bunch of those error codes should never happen (assuming your socket code works). There are error messages that should never happen here (caused by bugs in the code (bad file descriptor)) and there are error messages that are transient (network cable was unplugged). You should distinguish between the two types in your errors.

Which this code seems to do. But this is supposed to be low level socket code. It should not be handling protocols; this code should be in a derived class for the specific protocol that it understands.

##Self Plug I started writing a series of Blogs about Socket programming. Still not complete. But as part of that series you will find a socket wrapper class.

##Self Plug/ Further reading I started writing a series of Blogs about Socket programming. Still not complete. But as part of that series you will find a socket wrapper class.

I did not review the serverSock but most comments that apply to the client socket also apply to the server socket.

##Self Plug I started writing a series of Blogs about Socket programming. Still not complete. But as part of that series you will find a socket wrapper class.

• Socket Programming in C
• Socket Read/Write
• C++ Wrapper for Socket
• Socket Protocols

The code is:

• reviewed here - C++ Socket Part-2
• source here