Wrapping Curl, an HTTP client

I have written an HTTP client wrapped the libcurl. It should be able to do HTTP get/post with string/map param, with cookies and proxy.

Can somebody review the code? B.T.W., I'm not sure the way pass a map into HTTP header is correct, maybe I should remove these two interface

CURLcode do_http_post(std::string post_url, std::map map_param, void* user_data);

and

void set_http_header(std::map map_param);.

curl_wrapper.h

#ifndef CURL_WRAPPER
#define CURL_WRAPPER
#include 
#include 
#include "curl.h"
#include 
#define DATA_MAX_LEN CURL_MAX_WRITE_SIZE*30
struct client_data;

class curl_client{
public:
    curl_client();
    ~curl_client();
    CURLcode do_http_get(std::string get_url, void* user_data);
    CURLcode do_http_post(std::string post_url, std::map map_param, void* user_data);
    CURLcode do_http_post(std::string post_url, std::string post_fields, void* user_data);
    void set_http_header(std::string header_param);
    void set_http_header(std::map map_param);
    void set_http_cookie(std::string cookie_file);
    void set_http_proxy(std::string proxy_url);

private:
    static size_t write_data( char *ptr, size_t size, size_t nmemb, void *user_data);
    void set_common_opt(std::string url, void* user_data);
    void set_post_fields(std::string post_fields);
    void set_post_fields(std::map map_param);
    CURLcode perform();

private:
    CURL* curl_;
    CURLcode res_;
    curl_slist* p_header_list_;
};

struct client_data {
    client_data() {
        size = DATA_MAX_LEN;
        used = 0;
        buf = new char[DATA_MAX_LEN];
    }
    ~client_data(){
        if (buf!=nullptr){
            delete[] buf;
        }
    }
    char *buf;
    int size;
    int used;
};

#endif

curl_wrapper.cpp

```
#include "curl_wrapper.h"

curl_client::curl_client(){
curl_ = curl_easy_init();
p_header_list_ = nullptr;
}

curl_client::~curl_client(){
curl_easy_cleanup(curl_);
curl_slist_free_all(p_header_list_);
}

CURLcode curl_client::

Looking at your header file:

-
Replace the #define DATA_MAX_LEN with a static const variable; Do the same with any other constant that is #define-d.

-
Pass more complex parameters by const reference, to avoid making a copy (urls, parameters maps, etc).

-
You define client_data structure for (I assume) receiving the results; If it is strongly typed, why do you pass it in by void*? This just allows client code to call your API in a way that will corrupt your code (e.g. a client might decide to pass there a pointer to a std::vector instead).

-
You do not use RAII and smart pointers (you probably should)

-
After looking at your code, it is still unclear to me, if I would be able to use it to read the HTTP response headers (e.g. "I want to know if the response came with the "Cache-Control" header specified, and what was it's value").

-
Consider returning the result data as a return value (instead as an output parameter) and raising an exception in case you get a HTTP error. This would allow you to specify other error conditions as well.

-
Just looking at the interface (not the implementation) I have no idea how your class will behave if I call with invalid parameters.

Code:

curl_client curl;
client_data user_data;
curl.set_http_cookie("\\"); // does this throw? What does it throw?

Looking at your implementation file:

-
Your perform function calls curl_easy_perform (a C function) inside a try/catch block for std::exception. Exceptions are a C++ thing (i.e. curl_easy_perform will never throw an exception -- that's why it's using return codes to signal errors).

-
You are using C-style casts; Don't! (see my point above about removing the void* code).

-
There is no reason at all to use raw pointer and memcpy. Consider using std::vector instead (it will be safer, more efficient, exception-safe and already tested).

-
set_post_fields concatenates strings into a different string, in a loop. Consider using a std::ostringstream instead.

StackExchange Code Review Q#42121, answer score: 4