libcurl wrapper (tear it to shreds)

Ok, working on writing better code, and creating samples of my work so I can perhaps get back into this field. I got my libcurl wrapper working for what I needed it for. So before I move on to adding some extra features to it that are common requests for libcurl wrappers, I would you guys to tear it up and tell me everything I did wrong, or at least ask me to explain why I did something one way when you can think of a better way.

I want to be able to take this code to interviews (once I expand it a bit) so be harsh on it.

Header:

Code:

#ifndef CURLWRAPPER_H
#define CURLWRAPPER_H

#include <curl/curl.h>
#include <string>
namespace Wraithan {

size_t writer(const char *data, size_t size, size_t nmemb, std::string *buffer);

class curlwrapper {
    public:
        static curlwrapper* getInstance();
        ~curlwrapper();
        void addPost(const std::string &name,const std::string &content);
        std::string fetchPage(const std::string &url,int usepost=0);
        const std::string lastError();
        void setCookiesFile(const std::string &cookieFile = "cookie.jar");
        void destroyInstance();

    private:
        curlwrapper();
        curlwrapper(const curlwrapper &c) {};
        curlwrapper& operator=(const curlwrapper& c) { return *this; }
        CURL *handle;
        curl_httppost* post;
        curl_httppost* last;
        std::string writeBuffer;
        char errorBuffer[CURL_ERROR_SIZE];
        bool useCookies;
        static curlwrapper* instance;

};


} //namespace Wraithan
#endif // CURLWRAPPER_H

CPP:

Code:

#include "curlwrapper.h"

namespace Wraithan {

size_t writer(const char *data, size_t size, size_t nmemb, std::string *buffer) {
    size_t result = 0;

    if (buffer != NULL) {
        buffer->append(data,size*nmemb);
        result = size * nmemb;
    }

    return result;
}

//public:

curlwrapper* curlwrapper::getInstance() {
    if(instance == 0) {
        instance = new curlwrapper;
    }
    return instance;
}

curlwrapper::~curlwrapper() {
    /* Clean up everything */
    curl_easy_cleanup(handle);
    curl_global_cleanup();
}

void curlwrapper::addPost(const std::string &name,const std::string &content) {
    curl_formadd(&post, &last,
                 CURLFORM_COPYNAME, name.c_str(),
                 CURLFORM_COPYCONTENTS, content.c_str(),
                 CURLFORM_END);
    return;
}

std::string curlwrapper::fetchPage(const std::string &url,int usepost) {
    if(usepost != 0) {
        curl_easy_setopt(handle,CURLOPT_HTTPPOST,post);
    }
    curl_easy_setopt(handle,CURLOPT_URL,url.c_str());

    if(curl_easy_perform(handle) == CURLE_OK) {
        if(usepost != 0) {
            curl_formfree(post);
        }
        std::string retval(writeBuffer);
        writeBuffer = "";

        return retval;
    } else {
        return "";
    }
}

const std::string curlwrapper::lastError() {
    return errorBuffer;
}

void curlwrapper::setCookiesFile(const std::string &cookieFile) {

    curl_easy_setopt(handle,CURLOPT_COOKIEFILE,cookieFile.c_str());
    curl_easy_setopt(handle,CURLOPT_COOKIEJAR,cookieFile.c_str());
    useCookies = true;
    return;
}

void curlwrapper::destroyInstance() {
    delete instance;
    return;
}

//Private:
curlwrapper::curlwrapper() {
    /* Initiate libcurl */
    curl_global_init(CURL_GLOBAL_ALL);

    /* Get connection handle */
    handle = curl_easy_init();

    /* Set all standard options */
    curl_easy_setopt(handle,CURLOPT_ERRORBUFFER,&errorBuffer);
    curl_easy_setopt(handle,CURLOPT_HEADER,0);
    curl_easy_setopt(handle,CURLOPT_FOLLOWLOCATION,1);
    curl_easy_setopt(handle,CURLOPT_WRITEFUNCTION,writer);
    curl_easy_setopt(handle,CURLOPT_WRITEDATA,&writeBuffer);

    /* NULL out post variables */
    post = NULL;
    last = NULL;
    errorBuffer[0]='\0';
    writeBuffer = "";
}

curlwrapper* curlwrapper::instance = 0;


} //namespace Wraithan

Also, this works so if someone needed a basic C++ wrapper for libcurl, here ya go!

Code:

int writer(char *data, size_t size, size_t nmemb, std::string *buffer);

data is the data to be written, so it is better of as a const char*.

Code:

void addPost(std::string name,std::string content);
void setCookiesFile(std::string cookieFile = "cookie.jar");

Why did you (deliberately) not pass by const reference for these member functions?

Code:

std::string lastError();

This looks like it should be a const member function, though it probably does not matter since a const curlwrapper object is not particularly useful.

Code:

result = size * nmemb;

size and nmemb are size_t, but result is an int. Methinks you want result to be a size_t too.

EDIT:
Another thing comes to mind. What happens if someone copies a curlwrapper object?

Well fixed the first 4 things you pointed out, the first one I was wary of since that function is supposed to fit a certain prototype

Code:

size_t function( void *ptr, size_t size, size_t nmemb, void *stream);

so I didn't know if const'ing it would be safe, but it seems to still work. The lack of const & on some of those strings was an oversight, I forgot to write it that way. And good catch on the size_t stuff. I guess I am pretty sloppy =(

About the copy constructor, I should probably just privatize that, since curl handles shouldn't be copied since the options go with the handle, not the class so it would cause some issues. The biggest thing I just thought about though, is that

Code:

curl_global_init(CURL_GLOBAL_ALL);
curl_global_cleanup();

should only ever be called once in a program.

Originally Posted by Wraithan

About the copy constructor, I should probably just privatize that, since curl handles shouldn't be copied since the options go with the handle, not the class so it would cause some issues. The biggest thing I just thought about though, is that

Code:

curl_global_init(CURL_GLOBAL_ALL);
curl_global_cleanup();

should only ever be called once in a program.

That suggests the singleton pattern.

Ah good point, I have never implemented a singleton so they seem a little scary, but I will give it a go.

EDIT: Made a noob mistake putting a * in the wrong spot. Removed erroring code.

OP update, anyone else want to poke more holes?

I don't like the name "curlwrapper" at all. OK, so you wrap cURL, but what does the class actually do?

The singleton was a bad suggestion. Or perhaps it was misinterpreted. But I'm leaning towards the former. A singleton is for cases where there can logically only be one instance of the class. This is not the case here. There can be multiple requests, and by writing a singleton, you're merely inviting chaos.

Also, by making your class a singleton because cURL wants a single init and deinit call, you're leaking implementation details. Because actually, I don't care that you wrap cURL. I care about making web requests.

To solve the initialization issue, what you really needs is a init-once flag. Whenever you create an instance of the class, check the flag. If not set, set it, call the init function, and register the deinit function with atexit() or something like that. Or create a secret singleton that does the initialization. (That's the misinterpretation clause above.)

The default parameter on setCookieFile is misplaced. The property might have a default value in the class (but I'm leaning towards not giving it one and defaulting to rejecting all cookies instead, as that's the safe thing to do), but if I call setCookieFile, it's because I know where I want to store my cookies. Oh, and .jar is probably not a good extension. Unless cURL really stores cookies in a JAR archive.

Basically, I imagine such a library as being focused on a request object. I create it, supplying the target URL. I can add headers and content blocks. A special content block is POST data - basically a std::map-like thing that I add parameters to, and then push at the request. You could probably use std::map directly.
The I submit the request. I get back a response object. From the response object, I can read status, headers, and content. Request and response object can be the same. That makes implementation easier, but naming more difficult. I'm not sure which the better interface is - I'd have to try it. Separating them gives you more flexibility - for example, you could add an asynchronous mode which returns a future<response> (futures are high-level concepts for asynchronous operations).

OK, that's my comments on the design. More in the line of blowing everything up instead of poking some holes.

Here's some smaller technical issues.

Remove the implementations of copy constructor and assignment. You made them private because you want the thing to be non-copyable. There's no need for implementations.

Use an initializer list in the constructor. It's good style.

Are the explicit non-argument returns really necessary? Do you think they increase readability?

I think that's it, pretty much. Can't think of anything else right now.

I was sort of thinking that also, that singleton isn't the best choice. My problem was that I didn't know a good way to clean it up at the end because I don't want to risk calling the cleanup before the user was done with the library. I will look into the atexit() stuff, as I remember reading about it a long time back but would have to research it to make sure I use it right.

As far as the default name for the cookie jar/file I agree that the default name is not a good choice and will fix that, but you must explicitly call setCookiesFile in order to process cookies at all, otherwise cookies are ignored. So I feel I am rejecting all cookies by default with the class.

I hadn't planned on adding support for asynchronous, since it isn't something I could see myself needing. The support for accepting a std::map to build the POST is something I believe twomers brought up to me, but I felt that as a extra feature, not something I needed to be able to get this code off the ground (aka falls into would likes, instead of would needs). Along with making the cookies, header, and response all available separately.

Good point on the copy/assignment stuff, I will remove the implementations.

Initializer lists are something I never really paid attention to using, I always just put everything in the constructor, but no time like now to start good practices.

And the explicit non-argument returns are a matter of style I guess. I always put return in every function, even if it is a void function. To me that signifies the end of the function as much, or even more so than the losing }

CornedBee: Thank you very much for this insight, I will take the time to go over it more and apply everything I think needs to be right now, though some of your suggestions may be left as TODO features.

The singleton was a bad suggestion. Or perhaps it was misinterpreted. But I'm leaning towards the former. A singleton is for cases where there can logically only be one instance of the class. This is not the case here. There can be multiple requests, and by writing a singleton, you're merely inviting chaos.

Actually, I was thinking of wrapping the two calls in a singleton (i.e., there is only one instance of the global curl state). The wrapper itself should not be a singleton, since it makes sense to have multiple objects. Alternatively, one can use of the idea of a singleton, but allow multiple objects, controlled similiarly as a singleton.