1
\$\begingroup\$
#include <sys/types.h>
#include <sys/socket.h>

#include <netdb.h>
#include <string.h>
#include <unistd.h>

#include <string>
class Website
{
    int status, sock;
    struct addrinfo hints;
    struct addrinfo *servinfo;
    public:
      int init(std::string url){
          memset(&hints, 0, sizeof hints);
          hints.ai_family = AF_UNSPEC;
          hints.ai_socktype = SOCK_STREAM;
      if((status = getaddrinfo(url.c_str(), "80", &hints, &servinfo)) != 0){
              return status;
          }
          if((sock = socket(servinfo->ai_family, servinfo->ai_socktype, servinfo->ai_protocol)) == -1){
              return -1;
      }
          return 0;
       } 
     int connectToSite(){
        return connect(sock, servinfo->ai_addr, servinfo->ai_addrlen);
     }
     int sendToSite(std::string request){
        return send(sock, request.c_str(), strlen(request.c_str()), 0);
     }
     int recvFromSite(char buf[], int maxsize){
        return recv(sock, buf, maxsize, 0);
     }
    void closeSocket(){
      close(sock);
      freeaddrinfo(servinfo);
     } 
};

I have a networking class that implements low level c functions in a simple class. I am looking to make my code more efficient

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Is this the real indentation you are using, or was there a copy and paste error (where there some tabs in your code)? It might also be helpful if you provided a test case. \$\endgroup\$
    – pacmaninbw
    Oct 7 '20 at 18:08
3
\$\begingroup\$

Observation:

I don't like the design.

You are using a two phase initialization. You construct the object (thus constructor) and then call an init() function. Two phase initialization is a bad idea as you can't tell (or have to validate) the state of the object before it can be used.

A better technique is to simply use the constructor to initialize the object. If the constructor fails then the throw so the object never exists and can't be used incorrectly.

You also have an explicit close (closeSocket()) which is fine to an extent but you are forcing people to remember to call this function when you are done with the object. This is what a destructor is for.

You need to look up the concept of RAII.

{
    Website    site("Stuff"); // All resources allocated
}
// All resources for `site` are now released.

Design

What is the use of connectToSite(), is that not the init? Can you call read/write before calling this? If not then read/write are on the wrong object and connect should return you the appropriate object that has the read/write interface.


This is how I would want to use an object called "WebSite"

{
    // An abstract object that represents the site.
    // No actual underlying connection as connections are
    // usually to resources on the site not the actual site.
    Website    site("thorsanvil.com", 80);

    // Create a connection to a specific resource on the site.
    Connection c = site.open("Path");

    // Object was created successfully so we have
    // a valid connection to a server we can not communicate with.
    c.writeData("This is a message");
    std::string resp = c.read();
 }

From your interface. I would look to rename your class to "Connection" then the expectations for how to use the object may be more like.

 {
    // Create the socket and connect to the site.
    Connection   c("https://thorsanvil.com/MyResource");

    // Object was created successfully so we have
    // a valid connection to a server we can not communicate with.
    c.writeData("This is a message");
    std::string response = c.readData();
 }

Code Review:

You don't want to use C++ strings?

#include <string.h>

A URL?

  int init(std::string url)

Sure but the name of the class is "Website". A url is a lot more than just a website. There is a schema/host(hostname/port/user/password)/path/query/fragment. Would you not just pass a host and for the connection maybe a port.

Or you could pass the whole url but each new connection needs a new URL not just the website.


You are returning the status code here (on error):

  if((status = getaddrinfo(url.c_str(), "80", &hints, &servinfo)) != 0){
          return status;
      }

But here you are retuning -1

      if((sock = socket(servinfo->ai_family, servinfo->ai_socktype, servinfo->ai_protocol)) == -1){
          return -1;
      }

Are you sure those error ranges do not overlap?

I think a better interface would be to throw an exception if something goes wrong.


This is not going to work for the generic URL.

  status = getaddrinfo(url.c_str(), "80" ....
  1. The port is not always 80.
  2. The default port is defined by the schema "http/https ..."
  3. The default can be overridden by the url itself.

Example:

  https://thorsanvil.com:1222/Work

  Schema:   https
  Site:     thorsanvil.com
  Port:     1222             overrides default 443 for https
  Path:     /Work
  Query:    Empty
  Fragment: Empty

Really the above code should be:

  status = getaddrinfo(getHost(url), getPort(url), ....

What happens of connect() fails? Are you expecting the user to check the error code?

 int connectToSite(){
    return connect(sock, servinfo->ai_addr, servinfo->ai_addrlen);
 }

That's very C like interface and very error prone to use. We know that users of API don't always check error codes. Don't leak this ingormation. Make it explicit with an exception.


This is not how you use send() and recv() the return value indicates how much of the message was sent/recved. You need to repeatedly call this functions until the message is sent or there is an error.

 int sendToSite(std::string request){
    return send(sock, request.c_str(), strlen(request.c_str()), 0);
 }
 int recvFromSite(char buf[], int maxsize){
    return recv(sock, buf, maxsize, 0);
 }

Research

Have a look at this class:

https://github.com/Loki-Astari/ThorsSocket/blob/master/src/ThorsSocket/Socket.h

\$\endgroup\$
1
  • \$\begingroup\$ Thank you very much. This is very useful. \$\endgroup\$ Oct 8 '20 at 14:46

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.