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.