3
\$\begingroup\$

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();
}
\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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