I've written this class that downloads an images, the URLs for which are read from a local .txt file. I'm keen to make sure my code is safe and efficient.

#include <iostream>
#include <fstream>
#include <string>
#include <vector>
#include <algorithm>
#include <numeric>
#include <functional>
#include <curl/curl.h>

class DownloadTesting
    CURL *curl;
    FILE *file;
    CURLcode response;
    char outfilename[FILENAME_MAX];
    int count {0};
    std::vector<char*> URLS; //Stores the urls
    auto DownloadImage(const char *url) -> void;
    auto Convert(const std::string &s) -> char*;
    auto ReadDataStore() -> void;
    static auto WriteData(void *ptr, size_t size, size_t nmemb, FILE *stream) -> size_t;
    auto Interface() -> void;


   if(!URLS.empty()) //Delete raw pointers
     std::for_each(URLS.begin(),URLS.end(),[](char* url){delete url;});

auto DownloadTesting::WriteData(void *ptr, size_t size, size_t nmemb, FILE *stream) -> size_t //Write data into file 
   size_t written {fwrite(ptr, size, nmemb, stream)};
   return written;

auto DownloadTesting::Convert(const std::string &s) -> char* //Convert url from std::string to raw char pointer
  char *pc {new char[s.size()+1]};
  return pc;

auto DownloadTesting::ReadDataStore() -> void
  std::vector<std::string> storage_vector;
  std::string storage_string;
  std::ifstream storage_file {"./imagestore/urlstore.txt"};
  if(storage_file.is_open()) //Open local file
     while(getline (storage_file,storage_string)) //Read each line
       storage_vector.push_back(storage_string); //Storage each line in vector
  std::transform(storage_vector.begin(), storage_vector.end(), std::back_inserter(URLS), [this](const std::string url){return this->Convert(url);}); //Perform conversion function on each url

auto DownloadTesting::DownloadImage(const char *url) -> void
  count++; //Use a counter to name each downloaded file
  std::string name {std::to_string(count)}; //Convert int to string
  strcpy(outfilename, name.c_str()); //Convert std::string to [char]

  curl = curl_easy_init(); //Initialise curl
    file = fopen(outfilename,"wb");
    curl_easy_setopt(curl, CURLOPT_URL, url);
    curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, &DownloadTesting::WriteData);
    curl_easy_setopt(curl, CURLOPT_WRITEDATA, file);
    response = curl_easy_perform(curl);
    curl_easy_cleanup(curl); //Clean up curl
    fclose(file); //Close file

auto DownloadTesting::Interface() -> void
  this -> ReadDataStore(); //Read in the data
  assert(!URLS.empty()); //Ensure data is not empty
  std::for_each(URLS.begin(),URLS.end(), [this](const char* url){return this -> DownloadImage(url);}); //Perform download for each url

int main()
  DownloadTesting DTO;

1 Answer 1


Language Features

You are trying to use new C++ language features, but you appear to be using them inefficiently.

  • This:

    std::for_each(URLS.begin(),URLS.end(), [this](const char* url){return this -> DownloadImage(url);});

    can be replaced by a simple:

    for (auto& url : URLS) DownloadImage(url)
  • Using auto for method declarations only to specify the return type at the end of the line is confusing.

Memory Management

Rule of 5: a class which needs a custom destructor for dealing with resource (de)allocations, most likely also needs a custom copy constructor, assignment operator and move constructor/assignment operator.

You are creating raw copies of strings only to store raw pointers to the characters. Simply use string.c_str() when you need to pass a raw pointer to the library and let the string class manage the memory.


Writing to the file via the callback and also asking curl to write the data to a file looks like duplicate work. Any reason for this?


The class isn't a good example of object-oriented design. The only available public method, Interface, doesn't indicate what it might do. The entire code might have been better off as a single function.


Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.