How Can Improve This Simple C++ Winsock Code? - Criticize My Code!

[f ben isaac]

This is a simple client/server code (i posted the client code). I tried to let my code tells about itself by putting descriptive names and reducing comments.

How good this code in terms of clarity, readability, and overall design? Is it easy to grasp? How can i improve it?

Disregard input validation and system("pause"), cin.get() should be in there instead of system().

/**program description:
 *
 * this is a simple client-server
 * program. This client application
 * connects to a server, and then
 * user should enter some text. Text
 * is sent to the server and server
 * echos back the msg to the client.
 * Works under Windows OS. Connection
 * is based on TCP
 *
 */
 
#include<iostream>
#include<string>
#include<sstream>
#include<Ws2tcpip.h> 
using namespace std;
 
#define MAX_DATA 1024           //maximum bytes could be sent or received 
 
struct addrinfo hints;          //inputs connection type and ip type 
struct addrinfo *serverInfo;    //holds full address information of a server
SOCKET socketFileDescriptor;
char userInput[MAX_DATA];      
char serverResponse[MAX_DATA];
string ip;
string port;
 
void initiateWinsockFacility()
{
    WORD wVersionRequested;
    WSADATA wsaData;
    int error;
 
    wVersionRequested = MAKEWORD(2, 2);
 
    error = WSAStartup(wVersionRequested, &wsaData);
 
    if (error != 0) 
        {
        cout<<"WSAStartup failed with error: "<<error<<endl;
 
        system("pause");
        exit(-1);
    }
}
 
//output, to serverInfo, full information about the connection to be made
void getFullAddressInformation(const char *ipAddress, const char *portNumber)
{
        int status = getaddrinfo(ipAddress, portNumber, &hints, &serverInfo);
        
        if(status != 0)
        {
                cout<<"getaddrinfo() error: "<< gai_strerror(status) <<endl;
 
                WSACleanup();
 
                exit(-1);
        }
}
 
void socketError()
{
        cout<<"socket creation failed! ";
        cout<<"WSAGetLastError(): "<< WSAGetLastError() <<endl;
 
        freeaddrinfo(serverInfo);
        WSACleanup();
 
        system("pause");
        exit(-1);
}
 
void connectError()
{
        cout<<"connection failed! ";
        cout<<"WSAGetLastError(): "<< WSAGetLastError() <<endl;
 
        closesocket(socketFileDescriptor);
        freeaddrinfo(serverInfo);
        WSACleanup();
 
        system("pause");
        exit(-1);
}
 
void sendError()
{
        cout<<"message could not be sent! ";
        cout<<"WSAGetLastError(): "<< WSAGetLastError() <<endl;
 
        closesocket(socketFileDescriptor);
        freeaddrinfo(serverInfo);
        WSACleanup();
 
        system("pause");
        exit(-1);
}
 
void recvError()
{
        cout<<"message could not be received! ";
        cout<<"WSAGetLastError(): "<< WSAGetLastError() <<endl;
 
        closesocket(socketFileDescriptor);
        freeaddrinfo(serverInfo);
        WSACleanup();
 
        system("pause");
        exit(-1);
}
 
 
string convertIntToString(int number)
{
        string stringOutput;
        
        stringstream ss;
        ss << number;
        ss >> stringOutput;
 
        return stringOutput;
}
 
int convertStringToInt(string stringStream)
{
        int integerOutput;
 
        stringstream ss;
        ss << stringStream;
        ss >> integerOutput;
 
        return integerOutput;
}
 
int convertConstCharToInt(const char *characters)
{
        int integerOutput;
        string stringFromConstChar(characters);
        
        stringstream ss;
        ss << stringFromConstChar;
        ss >> integerOutput;
        
        return integerOutput;
}
 
int main(void)
{
        initiateWinsockFacility();
 
        system("cls");
 
        memset(&hints, 0, sizeof(hints));
 
        hints.ai_family = AF_INET;
        hints.ai_socktype = SOCK_STREAM;
 
        cout<<"Please enter server's IP: ";
        cin>>ip;
 
        cout<<"Please enter port's number: ";
        cin>>port;
 
        getFullAddressInformation(ip.c_str(), port.c_str());
 
        socketFileDescriptor = socket(serverInfo->ai_family, serverInfo->ai_socktype, serverInfo->ai_protocol);
        if(socketFileDescriptor == INVALID_SOCKET )
        {
                socketError();
        }
 
        cout<<"connecting to the server ..."<<endl;
 
        int connectStatus = connect(socketFileDescriptor, serverInfo->ai_addr, serverInfo->ai_addrlen);
        if(connectStatus == SOCKET_ERROR)
        {
                connectError();
        }
 
        cin.ignore();
 
        //send and receive messages
        while(true)
        {
                cout<<"\nyou: ";
                cin.getline(userInput, MAX_DATA);
                if(cin.fail())
                {
                        cout<<"Overflow occured. Restart Application"<<endl;
 
                        closesocket(socketFileDescriptor);
                        freeaddrinfo(serverInfo);
                        WSACleanup();
 
                        system("pause");
                        exit(-1);
                }
 
                int bytes_sent = send(socketFileDescriptor, userInput, strlen(userInput), 0);
                if(bytes_sent == SOCKET_ERROR)
                {
                        sendError();
                }
 
                int bytes_received = recv(socketFileDescriptor, serverResponse, MAX_DATA, 0);
                if(bytes_received == SOCKET_ERROR)
                {
                        recvError();
                }
 
                string echoedMsg(serverResponse, bytes_received);
                cout<<"server: "<< echoedMsg <<endl;
        }
 
        closesocket(socketFileDescriptor);
        freeaddrinfo(serverInfo);
        WSACleanup();
 
        system("pause");
        return 0;
}

[writetonsharma]

1. whenever you use any variable or pointer, first check if its null. its a small program but in big programs not checking passed parameters may result in crashes. you cannot expect that all the passes parameters are valid.

2. i always think if we are working in C then we should only use C. so converting char to int may be good if we use atoi.. it might give a better performance also..

3. rather than relying on global variables it good if you pass them as parameters to the functions. like for eg.

void socketError()
{
        cout<<"socket creation failed! ";
        cout<<"WSAGetLastError(): "<< WSAGetLastError() <<endl;
 
        freeaddrinfo(serverInfo);
        WSACleanup();
 
        system("pause");
        exit(-1);
}

you are expecting that serverInfo will be set befor this function is called!! but if this case fails what will happen?? ok leave that thing also.. if you port your code then a global variable with the name of serverInfo should be there and then only this function will work else it will break..

had it been like this:

void socketError(serverInfo sInfo)
{
        cout<<"socket creation failed! ";
        cout<<"WSAGetLastError(): "<< WSAGetLastError() <<endl;
 
        freeaddrinfo(sInfo);
        WSACleanup();
 
        system("pause");
        exit(-1);
}

now this code is independent.. can be ported anywhere.. make sense??

[f ben isaac]

Good tips, it helped...

2. i always think if we are working in C then we should only use C. so converting char to int may be good if we use atoi.. it might give a better performance also..

I used atoi at first, but its C style. I wanted to make my code all in C++.

Some developers say, errors shouldn't be sent to stdout! then how can the regular user tell the developer what error has occurred?

Thanks

[writetonsharma]

you can send errors to stdout or to a log file.

I wanted to make my code all in C++.

if this is the case you should use classes and not functions. your code is c, using cout or std::string doesnt make it c++.
anyway.. first make your code working and bug free then think of porting it to classes.

[S6k9GNh0]

In the sense that making all your code C++ is simply not using the built in functions that are available in C in C++ is somewhat retarded. Either way, atoi gives an immediate performance boost over the method that you chose. So it's actually pointless for you to do other wise.

[f ben isaac]

>>In the sense that making all your code C++ is simply not using the built in functions that are available in C in C++ is somewhat retarded.

Every one says something. I know many, say the opposite... Confusing me!