patterncppModerate
Stream that opens an HTTP GET and then acts like a normal C++ istream
Viewed 0 times
streamlikenormalgetthathttpthenactsandistream
Problem
Needed a quick stream to get JSON objects.
```
#ifndef THORSANVIL_SIMPLE_STREAM_THOR_STREAM_H
#define THORSANVIL_SIMPLE_STREAM_THOR_STREAM_H
#include
#include
#include
#include
#include
#include
namespace ThorsAnvil
{
namespace Stream
{
extern "C" size_t writeFunc(char ptr, size_t size, size_t nmemb, void userdata);
extern "C" size_t headFunc(char ptr, size_t size, size_t nmemb, void userdata);
class IThorStream;
class IThorSimpleStream: public std::istream
{
friend class IThorStream;
struct SimpleSocketStreamBuffer: public std::streambuf
{
typedef std::streambuf::traits_type traits;
typedef traits::int_type int_type;
SimpleSocketStreamBuffer(std::string const& url, bool useEasyCurl, bool preDownload, std::function markStreamBad)
: empty(true)
, open(true)
, sizeMarked(false)
, droppedData(false)
, preDownload(preDownload)
, sizeLeft(0)
, markStreamBad(markStreamBad)
{
curl = curl_easy_init();
if(!curl)
{ markStreamBad();
}
curl_easy_setopt(curl, CURLOPT_URL, url.c_str());
curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1L);
curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, writeFunc);
curl_easy_setopt(curl, CURLOPT_HEADERFUNCTION, headFunc);
curl_easy_setopt(curl, CURLOPT_WRITEHEADER, this);
curl_easy_setopt(curl, CURLOPT_WRITEDATA, this);
curl_easy_setopt(curl, CURLOPT_PRIVATE, this);
if (useEasyCurl)
{
/ Perform the request, res will get the return code /
CURLcode result = curl_easy_perform(curl);
if ((result != CURLE_OK) && (result != CURLE_WRITE_ERROR))
{ markStreamBad();
}
}
}
```
#ifndef THORSANVIL_SIMPLE_STREAM_THOR_STREAM_H
#define THORSANVIL_SIMPLE_STREAM_THOR_STREAM_H
#include
#include
#include
#include
#include
#include
namespace ThorsAnvil
{
namespace Stream
{
extern "C" size_t writeFunc(char ptr, size_t size, size_t nmemb, void userdata);
extern "C" size_t headFunc(char ptr, size_t size, size_t nmemb, void userdata);
class IThorStream;
class IThorSimpleStream: public std::istream
{
friend class IThorStream;
struct SimpleSocketStreamBuffer: public std::streambuf
{
typedef std::streambuf::traits_type traits;
typedef traits::int_type int_type;
SimpleSocketStreamBuffer(std::string const& url, bool useEasyCurl, bool preDownload, std::function markStreamBad)
: empty(true)
, open(true)
, sizeMarked(false)
, droppedData(false)
, preDownload(preDownload)
, sizeLeft(0)
, markStreamBad(markStreamBad)
{
curl = curl_easy_init();
if(!curl)
{ markStreamBad();
}
curl_easy_setopt(curl, CURLOPT_URL, url.c_str());
curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1L);
curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, writeFunc);
curl_easy_setopt(curl, CURLOPT_HEADERFUNCTION, headFunc);
curl_easy_setopt(curl, CURLOPT_WRITEHEADER, this);
curl_easy_setopt(curl, CURLOPT_WRITEDATA, this);
curl_easy_setopt(curl, CURLOPT_PRIVATE, this);
if (useEasyCurl)
{
/ Perform the request, res will get the return code /
CURLcode result = curl_easy_perform(curl);
if ((result != CURLE_OK) && (result != CURLE_WRITE_ERROR))
{ markStreamBad();
}
}
}
Solution
I have a few comments that are unrelated to the synchronous/asynchronous and/or header-only nature of the code.
parameters
bools
I don't like passing
differs from:
(...and the same for the other variations as well). I'd strongly prefer to create an enumeration and have the parameters of that type.
std::function
I don't like passing a
Then we modify the remainder to suit:
and:
Of course, if you're going to do this, you want to do it throughout (e.g., to the stream definition, not just the stream buffer definition).
Using these, defining an object looks more like this:
...which strikes me as quite a bit more self-explanatory.
userdata pointer
In
This prevents accidentally re-assigning
buffer manipulation
I also don't particularly like the code you used to add data to the end of the buffer:
I think I'd prefer to just insert the data in a single step:
lambda syntax
There's one other change I'd consider, but I'm a bit less certain about whether I'd actually make it. A lambda that takes no parameters doesn't need to include empty parens. Using this (and taking the preceding changes into account) the ctor for
Particularly given the degree to which people are undoubtedly accustomed to using empty parens when defining functions, this could lead to some confusion. I suspect when people are more accustomed to lambda syntax, we'll probably view the empty parens are kind of foolish looking, but for now it may be better to leave them there.
parameters
bools
I don't like passing
bools as parameters. I really dislike a function like your SimpleSocketStreamBuffer constructor that take multiple bools. You need to do a fair amount of looking to be sure how:foo x("www.google.com", false, true, bar);differs from:
foo x("www.google.com", true, false, bar);(...and the same for the other variations as well). I'd strongly prefer to create an enumeration and have the parameters of that type.
std::function
I don't like passing a
std::function as a parameter either. I'd prefer to see a template parameter to specify the function type, and then use an std::function only internally for storage. With those, the constructor looks something like this:enum class curl_type { easy, hard };
enum class dl_strategy { lazy, greedy };
template
SimpleSocketStreamBuffer(std::string const& url,
curl_type ct,
dl_strategy download,
func markStreamBad)Then we modify the remainder to suit:
, preDownload(download == dl_strategy::greedy)and:
if (ct == curl_type::easy)
// ...Of course, if you're going to do this, you want to do it throughout (e.g., to the stream definition, not just the stream buffer definition).
Using these, defining an object looks more like this:
foo x("www.google.com", curl_type::easy, dl_strategy::greedy, bar);...which strikes me as quite a bit more self-explanatory.
userdata pointer
In
writeFunc and headerFunc, it would probably be best to verify that userdata isn't a null pointer before using it. Once you've verified that it's non-null, I think I'd prefer to define owner as a reference instead of a pointer:SimpleSocketStreamBuffer & owner = *reinterpret_cast(userdata);This prevents accidentally re-assigning
owner to point anywhere else, and (arguably) simplifies the rest of the code a bit by letting you use . instead of -> when you're dealing with the owner object.buffer manipulation
I also don't particularly like the code you used to add data to the end of the buffer:
owner->buffer.resize(oldSize + bytes);
std::copy(ptr, ptr + bytes, &owner->buffer[oldSize]);I think I'd prefer to just insert the data in a single step:
owner->buffer.insert(owner->buffer.end(), ptr, ptr + bytes);lambda syntax
There's one other change I'd consider, but I'm a bit less certain about whether I'd actually make it. A lambda that takes no parameters doesn't need to include empty parens. Using this (and taking the preceding changes into account) the ctor for
IThorSimpleStream can end up looking like this:IThorSimpleStream(std::string const& url, dl_strategy s = dl_strategy::lazy)
: std::istream(NULL)
, buffer(url, curl_type::easy, s, [this]{this->setstate(std::ios::badbit); })
{
std::istream::rdbuf(&buffer);
}Particularly given the degree to which people are undoubtedly accustomed to using empty parens when defining functions, this could lead to some confusion. I suspect when people are more accustomed to lambda syntax, we'll probably view the empty parens are kind of foolish looking, but for now it may be better to leave them there.
Code Snippets
foo x("www.google.com", false, true, bar);foo x("www.google.com", true, false, bar);enum class curl_type { easy, hard };
enum class dl_strategy { lazy, greedy };
template <class func>
SimpleSocketStreamBuffer(std::string const& url,
curl_type ct,
dl_strategy download,
func markStreamBad), preDownload(download == dl_strategy::greedy)if (ct == curl_type::easy)
// ...Context
StackExchange Code Review Q#38402, answer score: 15
Revisions (0)
No revisions yet.