3
\$\begingroup\$

Please tell me if this function is correct (I believe it to be), efficient, and clean.

Function to check:

::CURLcode CURL_DownloadFile( __in ::LPCSTR URL, __in ::LPCSTR Destination )
{
    ::CURL* Curl;
    ::CURLcode Return_CurlCode;
    std::FILE* File;

    if( ( Curl = ::curl_easy_init( ) ) )
    {
        File = std::fopen( Destination, "wb" );

        ::curl_easy_setopt( Curl, CURLOPT_URL, URL );
        ::curl_easy_setopt( Curl, CURLOPT_WRITEFUNCTION, WriteData );
        ::curl_easy_setopt( Curl, CURLOPT_WRITEDATA, File );

        Return_CurlCode = ::curl_easy_perform( Curl );
        std::fclose( File );
    } else { ( Return_CurlCode = CURLE_FAILED_INIT ); }

    return( Return_CurlCode );
};

Example of usage:

#include "Global.h"

::CURLcode CURL_DownloadFile( __in ::LPCSTR URL, __in ::LPCSTR Destination );

int main( void )
{
    if( CURL_DownloadFile( "http://stackoverflow.com/", "c:\\users\\xorr\\desktop\\stackoverflow.txt" ) != CURLE_OK )
        std::printf( "An error has occured...\n" );

    std::cin.get( );
    return( EXIT_SUCCESS );
    // Xorr@Hotmail.com
};

size_t WriteData( void *ptr, size_t size, size_t nmemb, std::FILE *stream ) {
    size_t written;
    written = fwrite(ptr, size, nmemb, stream);
    return written;
}

::CURLcode CURL_DownloadFile( __in ::LPCSTR URL, __in ::LPCSTR Destination )
{
    ::CURL* Curl;
    ::CURLcode Return_CurlCode;
    std::FILE* File;

    if( ( Curl = ::curl_easy_init( ) ) )
    {
        File = std::fopen( Destination, "wb" );

        ::curl_easy_setopt( Curl, CURLOPT_URL, URL );
        ::curl_easy_setopt( Curl, CURLOPT_WRITEFUNCTION, WriteData );
        ::curl_easy_setopt( Curl, CURLOPT_WRITEDATA, File );

        Return_CurlCode = ::curl_easy_perform( Curl );
        std::fclose( File );
    } else { ( Return_CurlCode = CURLE_FAILED_INIT ); }

    return( Return_CurlCode );
};
\$\endgroup\$
  • \$\begingroup\$ One obvious thing that strikes me is ::CURLcode Return_CurlCode; is never initialized if ::curl_easy_init() returns false. \$\endgroup\$ – Matthew Jan 25 '13 at 3:10
  • \$\begingroup\$ @Matthew thanks! Fixed it. This is what I added: else { ( Return_CurlCode = CURLE_FAILED_INIT ); } \$\endgroup\$ – Xorr Jan 25 '13 at 3:47
3
\$\begingroup\$

Your write function is incorrect:

size_t WriteData( void *ptr, size_t size, size_t nmemb, std::FILE *stream ) {
    size_t written;
    written = fwrite(ptr, size, nmemb, stream);
    return written;
}

CURL will abort the transfer if you do not return size*nmemb. So you need to take care to make sure that you actually write all the data provided.

Also this is the action of the default CURLOPT_WRITEFUNCTION so there is no need to write your own version.

You don't check if the File pointer is NULL

    File = std::fopen( Destination, "wb" );

This is a C function (even if you have used it from C++). This means you must check the return value to make sure the faction succeeded.

Don't artifically stop the application. You may forget to remove this and then it will make the program look like it is broken.

std::cin.get( );

If you run from the command line it is not needed. If you are running inside an IDE set up your IDE so that it does not close the output window when the application finishes.

If your program always succeeded. Then I prefer not to return an exit value from main(). The compiler in this one special case will plant a return 0; for you to indicate success. This is a good indication that you have designed your application so that it never fails.

return( EXIT_SUCCESS );

I only return a value if there is a potential for failure and I want to be able to indicate the possible failure states.

\$\endgroup\$
  • \$\begingroup\$ Isn't a return 0 a good indication that something suceeds... ? This is more explicit and clearer to me than letting the compiler do it. \$\endgroup\$ – Quentin Pradet Feb 16 '13 at 10:20
  • \$\begingroup\$ @Cygal: No. If you have a return at the end of main() you are indicating that there is a possibility of a failure. With no return you are indicating that there is no error condition that will ever be returned by the application. \$\endgroup\$ – Martin York Feb 16 '13 at 16:37

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.