Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Please tell me if this function is working properly (which it should), efficient, and clean. Suggestions are accepted! :-)

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 );
};
share|improve this question
    
One obvious thing that strikes me is ::CURLcode Return_CurlCode; is never initialized if ::curl_easy_init() returns false. –  Matthew Jan 25 '13 at 3:10
    
@Matthew thanks! Fixed it. This is what I added: else { ( Return_CurlCode = CURLE_FAILED_INIT ); } –  Xorr Jan 25 '13 at 3:47
add comment

1 Answer 1

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.

share|improve this answer
    
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. –  Quentin Pradet Feb 16 '13 at 10:20
    
@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. –  Loki Astari Feb 16 '13 at 16:37
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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