Downloading images using CURL

Question

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<cassert>
#include <algorithm>
#include <numeric>
#include <functional>
#include <curl/curl.h>

class DownloadTesting
{
  private:
    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;
  public:
    DownloadTesting();
    ~DownloadTesting();
    auto Interface() -> void;
};


DownloadTesting::DownloadTesting(){};

DownloadTesting::~DownloadTesting()
{
   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]};
  std::strcpy(pc,s.c_str());
  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
     }
     storage_file.close();
  }
  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
  if(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;
  DTO.Interface();
}

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.

CURL

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?

OOP

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.