5
\$\begingroup\$

As a project to get more familiar with curl, I decided to write a program that downloads stock data from Yahoo Finance, and does arbitrary analysis on that data. I would like to receive any critiques about the way I went about this, especially setting up the curl "environment", along with any other C standards I might have missed.


downloader.h

/**
 * Data Downloader Implementation.
 * 
 * @author Ben Antonellis
*/

#include <curl/curl.h>
#include <curl/easy.h>
#include <string.h>
#include <stdio.h>
#include <time.h>

#define DOWNLOAD_PATH "/tmp/stock_data.txt"

int DateToUnix(char* date) {
    struct tm tm;
    time_t epoch;
    if (strptime(date, "%Y-%m-%d", &tm) != NULL) {
        epoch = mktime(&tm);
    }
    return epoch;
}

int FormatQueryString(char* result, char* ticker, char* start_time, char* end_time, char* interval) {
    char* res;
    time_t from = DateToUnix(start_time);
    time_t to = DateToUnix(end_time);
    if (0 > asprintf(
        &res,
        "https://query1.finance.yahoo.com/v7/finance/download/%s?period1=%ld&period2=%ld&interval=%s&events=history", 
        ticker,
        from,
        to,
        interval)) return 1;
    strcpy(result, res);
    return 0;
}

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

void DownloadData(char* url) {
    CURL* curl;
    FILE* file;
    CURLcode result;

    curl = curl_easy_init();
    if (!curl) {
        printf("Error in DownloadData.\n");
        exit(1);
    }

    file = fopen(DOWNLOAD_PATH, "wb");
    curl_easy_setopt(curl, CURLOPT_URL, url);
    curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, WriteData);
    curl_easy_setopt(curl, CURLOPT_WRITEDATA, file);
    result = curl_easy_perform(curl);
    //fprintf(file, "\n");
    curl_easy_cleanup(curl);
    fclose(file);

}

program.c

/**
 * This project downloads stock data from Yahoo Finance.
 * 
 * @author Ben Antonellis
*/

#include <stdio.h>
#include <stdlib.h>

#include "downloader.h"

int main(int argc, char** argv) {

    char* result = (char*)malloc(256);
    if (FormatQueryString(result, "SPY", "2020-09-01", "2020-10-01", "1mo") > 0) {
        printf("Error in FormatQueryString\n");
        exit(1);
    }
    DownloadData(result);
    DateToUnix("2020-09-01");

    return 0;

}

A specific question I have is about the malloc statement. I use 256 because that seems to be enough to fit the query string, but I'm not sure about the ramifications about allocating too much space for the query, or perhaps not enough.

\$\endgroup\$
1
  • \$\begingroup\$ Either tell FormatQueryString() to strncpy() at most BUF_LEN (256) bytes, or finesse the string lifecycle details of your API. \$\endgroup\$
    – J_H
    Commented Aug 24, 2023 at 19:50

3 Answers 3

5
\$\begingroup\$

Enable all compiler warnings

This saves you and others time.

  • warning: implicit declaration of function 'strptime'; did you mean 'strftime'? [-Wimplicit-function-declaration]
  • warning: comparison between pointer and integer
  • warning: conversion from 'time_t' {aka 'long int'} to 'int' may change value [-Wconversion]
  • warning: implicit declaration of function 'asprintf'; did you mean 'sprintf'? [-Wimplicit-function-declaration]
  • warning: implicit declaration of function 'exit' [-Wimplicit-function-declaration]
  • note: include '<stdlib.h>' or provide a declaration of 'exit'
  • warning: incompatible implicit declaration of built-in function 'exit' [-Wbuiltin-declaration-mismatch]
  • warning: variable 'result' set but not used [-Wunused-but-set-variable]

Check all logic paths

Indefinite value returned when strptime(...) == NULL

    time_t epoch;  // Consider initializing here
    if (strptime(date, "%Y-%m-%d", &tm) != NULL) {
        epoch = mktime(&tm);
    }
    return epoch;

Avoid naked magic numbers

Why 256 in char* result = (char*)malloc(256);? How does the following code know that is sufficient?

Instead, pass the size into and have that code error if the size is insufficient.

// FormatQueryString(result, "SPY",....
if (FormatQueryString(256, result, "SPY",...)) {
  Handle_Error();
}

Cast not needed

// char* result = (char*)malloc(256);
char* result = malloc(256);

Robust code also look for errors.

if (result == NULL) {
  TBD_Code();
}

Look for I/O failures

Add tests.

file = fopen(DOWNLOAD_PATH, "wb");
if (file == NULL) {  // add
  TBD_Code();
}

Remove dead code

//fprintf(file, "\n"); deserves to be deleted as part of a code review.

String deserves abstraction

Rather than embed such a string directly in code, use a macro or object.

"https://query1.finance.yahoo.com/v7/finance/download/%s?period1=%ld&period2=%ld&interval=%s&events=history"

Check for curl errors

curl_easy_setopt returns a value.
Check it.

\$\endgroup\$
4
\$\begingroup\$

The code leaks memory. asprintf allocates memory. You need to free(res) before returning from FormatQueryString, especially if you plan to call it multiple times. Better yet, don't malloc a result in main, and don't strcpy anything. Let asprintf do the allocation:

    int main(....)
    {
        char * queryString = FormatQueryString(....);
        if (result == NULL) {
            // Handle error
        } else {
            // proceed with happy path
            // don't forget to
            free(queryString);
        }
    }

    char * FormatQueryString(....)
    {
        char * result;
        asprintf(&result, ....);
        return result;
    }
\$\endgroup\$
3
  • \$\begingroup\$ A downside to asprintf() is that it is not part of the standard C library and post lacks a compiler specific flag. It may exist in curl environments. \$\endgroup\$ Commented Aug 25, 2023 at 20:45
  • \$\begingroup\$ @chux-ReinstateMonica In general, agree (the printf family is a total mess from the inception). OTOH since te OP already uses it, I wouldn't call it a downside. \$\endgroup\$
    – vnp
    Commented Aug 25, 2023 at 21:13
  • \$\begingroup\$ To be clear, we are commenting about asprintf() and not printf() and it lack of inclusion in the standard C library. Yes printf() does have its weaknesses too. \$\endgroup\$ Commented Aug 25, 2023 at 21:28
1
\$\begingroup\$

WriteData() should have static linkage:

As the function is only used in this translation unit, it should be declared with having internal linkage:

// size_t WriteData(void* ptr, size_t size, size_t n, FILE* stream) 

// Perhaps WriteDataCallback() might be a better option? */
static size_t WriteData(void* ptr, size_t size, size_t n, FILE* stream) 

restrict could allow for select optimizations:

// int FormatQueryString(char* result, char* ticker, char* start_time, char* end_time, char* interval) 

int FormatQueryString(char *restrict result, char *restrict ticker, char *restrict start_time, 
                      char *restrict end_time, char *restrict interval)

Arguments that are not supposed to be modified should be declared with the const qualifier:

An object of const-qualified is read-only.

// int FormatQueryString(char* result, char* ticker, char* start_time, char* end_time, char* interval)

int FormatQueryString(char *result, const char *ticker, char *start_time, 
                      char *end_time, const char *interval) {

Don't declare more variables than you require:

// size_t written = fwrite(ptr, size, n, stream);
// return written;

return fwrite(ptr, size, n, stream);

Declare variables where they are used:

#if 0
    CURL* curl;
    FILE* file;
    CURLcode result;

    curl = curl_easy_init();
    if (!curl) {
        printf("Error in DownloadData.\n");
        exit(1);
    }

    file = fopen(DOWNLOAD_PATH, "wb");
#else
    CURL *curl = curl_easy_init();
    if (!curl) {
        printf("Error in DownloadData.\n");
        exit(1);
    }

    FILE *file = fopen(DOWNLOAD_PATH, "wb");
#endif

Consider using EXIT_SUCCESS and EXIT_FAILURE from stdlib.h;

// exit(1);
exit (EXIT_FAILURE);

Use more const:

// CURL *curl;
// FILE *file;

CURL *const curl;
FILE *const file;

Casting time_t to the widest unsigned or signed type might help avoid undefined behavior:

#if 0
// Consider defining the URL as a macro, or reading it from a configuration file.
   asprintf(
        &res,
        "https://query1.finance.yahoo.com/v7/finance/download/%s?period1=%ld&period2=%ld&interval=%s&events=history", 
        ticker,
        from,
        to,
        interval)
#else 
    asprintf(
        &res,
        "https://query1.finance.yahoo.com/v7/finance/download/%s?period1=%jd&period2=%jd&interval=%s&events=history", 
        ticker,
        (intmax_t) from,
        (intmax_t) to,
        interval)

See:What is time_t ultimately a typedef to?

The inclusion of curl/easy.h is unnecessary:

#include <curl/curl.h>
// #include <curl/easy.h>

Aside:

You may wish to look into these two options:

CURLOPT_FOLLOWLOCATION -- Follow HTTP redirects. See CURLOPT_FOLLOWLOCATION

CURLOPT_MAXREDIRS -- Maximum number of redirects to follow. See CURLOPT_MAXREDIRS

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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