##Design
This is a very thin C like wrapper around sockets. In my opinion there are a lot of style changes that are need to make this good C++ or usable by modern C++ library.
Main issues:
* Two phase construction
* Error Codes escape public interface (use exception)
* You member variables have state not related to the object after construction.
* Your use of `select()` is incorrect for this context.
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](http://lokiastari.com/blog/2016/04/08/socket-programming-in-c-version-1/)
* [Socket Read/Write](http://lokiastari.com/blog/2016/04/09/socket-read/)
* [C++ Wrapper for Socket](http://lokiastari.com/blog/2016/05/26/c-plus-plus-wrapper-for-socket/)
* [Socket Protocols](http://lokiastari.com/blog/2016/05/29/socket-protocols/)
The code is:
* [reviewed here - C++ Socket Part-2](https://codereview.stackexchange.com/q/131137/507)
* [source here](https://github.com/Loki-Astari/Examples/tree/master/Version2)
##Code Review
###Don't use `using namespace`
using namespace std;
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?](https://stackoverflow.com/q/1452721/14065)
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.
###Public Member Variables are always a bad idea:
Anybody can come along and change the state of your object without you being able to tell.
class clientSock {
public:
.....
string host;
unsigned int port;
bool connected;
Member objects should always be private with access controlled via public/protected member methods.
###Never Use Two Phase Construction
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.
This is your second phase check.
bool hasError();
Don't do it. If the constructor fails then throw an exception.
###Error Codes.
Error codes are great inside your object to indicate error states internally. But error codes should never escape your public interface. Prefer to throw an exception rather than force the user of your object to check an error code to verify correctness.
These return error codes.
int connect(string host, unsigned int port);
int disconnect();
A user of your code will eventually forget to check an error code thus resulting in bug.
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).
###Semantics and Rule of Three
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.
class clientSock {
public:
clientSock(string host, unsigned int port);
clientSock();
clientSock(int sock);
~clientSock();
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.
###Interface design
string readAll();
Read everything? You have not defined a protocol at this level. So how do you know what everything is?
###Member Vs Local variable
These two are not really part of the state of the object.
struct sockaddr_in servaddr;
struct hostent* server;
They are used to set up the connection. But are not used after that point. So not really much point in keeping them as members.
###Initializer List
clientSock::clientSock(string host, unsigned int port) {
connect(host, port);
}
OK I am not going to say its totally wrong. But I think you should set up member variables in the constructor before calling member functions. Currently the state of the object is random (for POD values) and member functions assume (especially public facing ones) assume that the state of the object is well defined.
Now using member function to set the state is OK (if you have a couple of constructors and some large piece of common code). But I don't think this falls into the same category. So I would initialize the members then call `connect()`.
clientSock::clientSock(string host, unsigned int port)
: host("")
, port(-1)
, connected(false) // this is the important one as you check it in connect.
, sock(-1)
{
connect(host, port);
}
Good practice to initialize all members so they have defined values. In the next one `port` will have an indeterminate value.
clientSock::clientSock() {
connected = false;
}
This one shows that `host` and `port` are not really part of the state of the object. They should be removed from the object. As soon as you have a socket them they are no longer relevant.
clientSock::clientSock(int sock) {
sockfd = sock;
connected = true;
}
Why is the disconnect commented out?
clientSock::~clientSock() {
//disconnect();
}
Maybe you need to check if it is connected before disconnecting.
###Member accesses
clientSock::host = host;
clientSock::port = port;
This is a funny way of accessing member variables. Normally I would use `this->` to specify the exact member (rather than `className::` as that implies static members).
But better yet is not never to shadow member variables. Shadowed variables will eventually cause a problem as you will forget to disambiguify them from the shadow and your compile will not warn you when you go wrong.
A nice trick is to turn on your compiler warnings to tell you about shadowed variables and to treat them as errors.
###Comments
I hate bad comments. But this would be a nice place for a comment.
setsockopt(sockfd, SOL_SOCKET, SO_RCVTIMEO, (char*)&tv, sizeof(struct timeval));*/
enable_keepalive(sockfd);
###Magic Numbers
Put magic numbers (like 3) into constants.
for(size_t i = 0; i < 3; i++) { //try to connect 3 times
if(::connect(sockfd, (struct sockaddr*) &servaddr, sizeof(servaddr)) < 0)
cerr << "Error on connecting: " << errno << " " << strerror(errno) << endl;
else {
connected = true;
return 0;
}
}
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.
###Boolean results with if
if(retval != 0 || error != 0)
return true;
else
return false;
This can be much more logically represented as:
return (retval != 0 || error != 0);
###Is it really an error to try and disconnect when not connected?
int clientSock::disconnect() {
if(!connected)
return -1; // This is an error state?
close(sockfd);
connected = false;
return 0;
}
###Pass parameters by const reference (when they are non mutable objects).
int clientSock::write(string mesg) {
You just made a copy of `mesg`. If this is a large string then that is a waste of time since you will just be reading it.
int clientSock::write(string const& mesg) {
###Select
You are using `select()` incorrectly. It's not going to break anything as it is used here. But this is not what it is designed for.
FD_ZERO(&writefds);
FD_SET(sockfd, &writefds);
int rv = select(sockfd + 1, NULL, &writefds, NULL, &tv);
The `select()` blocks waiting for one of the file descriptors to become available so that a using it will not block when it is used.
But you are blocking on the `select()` so you are not gaining any advantage to using select here. The point of `select()` is when you have lots of sockets with lots of data being read/written. Then `select()` allows you to continuously read from sockets that are ready and not block waiting on sockets that have nothing yet.
Remove `select()` from this context.
###Printing error messages
This code is way to low level to be printing error messages.
if(rv == -1)
cerr << errno << " " << strerror(errno) << endl;
Error messages should be sent to the user by a piece of code that understands the context under which the code is being used. If this code is being used as a server nobody will ever see this message. If this code is being used as a windows application nobody will ever see this message.
Throw an exception if this is unrecoverable (with the message). Some higher level piece of code will get the message and put it in the appropriate place (log file std::cerr etc).
###Not all write errors are unrecoverable
if(sentBytes == -1) {
cerr << "Error sending IDs: " << errno << " " << strerror(errno) << endl;
return 1;
}
There are a couple of error messages from `::write` that are not unrecoverable.
https://linux.die.net/man/2/write
EAGAIN or EWOULDBLOCK or EINTR (EINTR being relatively common).
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.
###Success failure as 0/1?
C++ has a bool type. If you want to pass success/failure state use a bool rather than a number (unless that number has other meaning). And don't let an error code escape a public interface.
###Read All
This implies a protocol so you know where the end is.
string clientSock::readAll() {
string full = read();
while(full.find("END") == string::npos)
full += read();
full = full.substr(0, full.find("END"));
return full;
}
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.