C program for storing the time of the sunset and sunrise

Question

I just wrote my first C program for storing the time of the sunset and sunrise for a location given in latitude and longitude in a SQLite database. I usually write C# only. Since this runs on a RaspberryPi I decided to do it in C and I don't really need all that extra stuff that C++ has and I dislike writing anything in Python that is more than just a quick mockup.

I am not familiar with C and I only now the basics about pointers and memory allocation, which is why I want to make sure if I can improve things and do them the C way properly or at least halfway decent.

It is quite a lot of code (>400 lines) and I'm sorry for that. Writing in C was really fun and I want to do more projects in C only.

Does my code meet C99 standards?

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <math.h>
#include <sqlite3.h>
#include <time.h>
#include <stdbool.h>

// Mathematical constants for sunrise calculation.
#define PI 3.1415926
#define ZENITH -.83

// FILE constants for opening and reading files.
#define FILE_OK 0
#define FILE_NOT_EXIST 1
#define FILE_TOO_LARGE 2
#define FILE_READ_ERROR 3

// First and last month for date calculation.
#define FIRSTMONTH 1
#define LASTMONTH 12


/*
** Read the files contents and return them.
*/
char * read_file(const char *fname) {

    char * buffer = 0;
    long length;

    FILE * fp = fopen(fname, "rb");

    if (fp) {
        fseek(fp, 0, SEEK_END);
        length = ftell(fp);
        fseek(fp, 0, SEEK_SET);
        buffer = (char*)malloc((length+1)*sizeof(char));
        if (buffer) {
            fread(buffer, sizeof(char), length, fp);
        }
        fclose(fp);
    }
    buffer[length] = '\0';
    return buffer;
}


/*
** Replace part of string and return the new string. 
*/
char * str_replace(char *src, char *trg, char *with) {
    char *retVal;
    char *ins;
    char *tmp;
    int len_rep;
    int len_with;
    int len_front;
    int count;

    if (!src || !trg) {
        return NULL;
    }

    len_rep = strlen(trg);
    if (len_rep == 0) {
        return NULL;
    }

    if (!with) {
        with = "";      
    }
    len_with = strlen(with);

    ins = src;
    for (count=0; tmp=strstr(ins, trg); ++count) {
        ins = tmp + len_rep;
    }

    tmp = retVal = malloc(strlen(src) + (len_with - len_rep) * count + 1);
    if (!retVal) {
        return NULL;
    }

    while (count--) {
        ins = strstr(src, trg);
        len_front = ins-src;
        tmp = strncpy(tmp, src, len_front) + len_front;
        tmp = strcpy(tmp, with) + len_with;
        src += len_front + len_rep;
    }
    strcpy(tmp, src);
    return retVal;
}


/*
** Callback for sqlite3_exec() method.
** Isn't really used at all.
*/
static int callback(void *ignore, int argc, char **argv, char **azColName) {
    int i;
    for (i=0; i<argc; i++) {
        printf("%s = %s\n", azColName[i], argv[i] ? argv[1] : "NULL");
    }
    printf("\n");
    return 0;
}


/*
** Execute SQL command against database.
*/
int execute_command(char * command) {
    sqlite3 *db;
    char *zErrMsg = 0;
    char *path = "./solar_db.sqlite3";
    int rc;
    rc = sqlite3_open(path, &db);
    if (rc) {
        fprintf(stderr, "Can't open database: %s\n", sqlite3_errmsg(db));
        sqlite3_close(db);
        return (1);
    }
    rc = sqlite3_exec(db, command, callback, 0, &zErrMsg);
    if (SQLITE_OK != rc) {
        fprintf(stderr, "SQL error: %s\n", zErrMsg);
        sqlite3_free(zErrMsg);
    }
    sqlite3_close(db);
    return 0;
}


/*
** Format SQL command template and 
** run formatted command against database.
*/
void insert_entry(
    const char * insert,
    struct tm *date, 
    struct tm *sunrise, 
    struct tm *sunset, 
    float lat, 
    float lng, 
    int localOffset, 
    int daylightSavings) 
{
    char *template = (char*)insert;
    
    char datebuff[17];
    strftime(datebuff, 17, "%Y-%m-%d", date);
    template = str_replace(template, "{0}", datebuff);
    
    char timebuff[7];
    strftime(timebuff, 7, "%H:%M", sunrise);
    template = str_replace(template, "{1}", timebuff);

    strftime(timebuff, 7, "%H:%M", sunset);
    template = str_replace(template, "{2}", timebuff);

    int len = snprintf(NULL, 0, "%f", lat);
    char * fbuff = malloc(len+1);
    snprintf(fbuff, len+1, "%f", lat);
    template = str_replace(template, "{3}", fbuff);
    free(fbuff);

    len = snprintf(NULL, 0, "%f", lng);
    fbuff = malloc(len+1);
    snprintf(fbuff, len+1, "%f", lng);
    template = str_replace(template, "{4}", fbuff);
    free(fbuff);

    len = snprintf(NULL, 0, "%d", localOffset);
    char * nbuff = malloc(len+1);
    snprintf(nbuff, len+1, "%d", localOffset);
    template = str_replace(template, "{5}", nbuff);
    free(nbuff);

    len = snprintf(NULL, 0, "%d", daylightSavings);
    nbuff = malloc(len+1);
    snprintf(nbuff, len+1, "%d", daylightSavings);
    template = str_replace(template, "{6}", nbuff);
    free(nbuff);

    execute_command(template);
    
    //printf(template);
    return;
}


/*
** Calculate the sunrise or sunset for given date
** and location provided by latitude and longitude.
*/
float calc_sun_time(
    int year, int month, int day, 
    float lat, float lng, 
    int localOffset, 
    int daylightSavings, 
    int settingTime) 
{

    // Calculate day of the year.
    float N1 = floor(275*month/9);
    float N2 = floor((month+9)/12);
    float N3 = (1+floor((year-4*floor(year/4)+2)/3));
    float N = N1-(N2*N3)+day-30;

    // Convert longitude to hour value and approximate time.
    float lngHour = lng/15.0;
    float t = 0.0;
    if (settingTime != -1) {
        t = N+((18-lngHour)/24);
    } /* calculate setting time.  */
    else {
        t = N+((6-lngHour)/24);
    } /* calculate rising time. */

    // Calculate sun's mean anomaly.
    float M = (0.9856*t)-3.289;

    // Calculate sun's true longitude.
    float L = fmod(M+(1.916*sin((PI/180)*M)) + (0.020 * sin(2*(PI/180)*M)) + 282.634, 360.0);

    // Calculate sun's right ascension.
    float RA = fmod(180/PI*atan(0.91764*tan((PI/180)*L)), 360.0);

    // Right ascension value need to be in the quadrant as L.
    float Lquadrant = floor(L/90)*90;
    float RAquadrant = floor(RA/90)*90;
    RA = RA + (Lquadrant-RAquadrant);

    // Right ascension value needs to be converted to hours.
    RA = RA / 15;

    // Calculate the sun's declination.
    float sinDec = 0.39782*sin((PI/180)*L);
    float cosDec = cos(asin(sinDec));

    // Calculate the sun's local hour angle.
    float cosH = (sin((PI/180)*ZENITH)-(sinDec*sin((PI/180)*lat))) / (cosDec*cos((PI/180)*lat));

    float H = .0f;
    if (settingTime != -1) {
        H = (180/PI)*acos(cosH);
    }
    else {
        // Calculate H and convert to hours.
        H = 360-(180/PI)*acos(cosH);
    }
    H = H/15;

    // Calculate local mean time of rising/setting.
    float T = H+RA-(0.06571*t)-6.622;

    // Adjust back to UTC;
    float UT = fmod(T-lngHour, 24.0);

    return UT + localOffset + daylightSavings;
}


/*
** Get the amount of days for a given month.
** The year parameter is used to check for leap years
** and the amount of days in february.
*/
int days_in_month(int year, int month) {


    if (month == 1 || month == 3 || month == 5 || month == 7 || month == 8 || month == 10 || month == 12)    {
        return 31;
    }
    else if (month == 2) {
        if ((year %   4 == 0)
        &&  (year % 100 != 0)
        &&  (year % 400 == 0)) 
        {
            return 29;
        } /* Leap year. */
        else {
            return 28;
        } /* Not a leap year. */
    }
    else if (month == 4 || month == 6 || month == 9 || month == 11) {
        return 30;
    }
}


/*
** Get the day of the week.
** Values range from 0 to 6;
*/
int day_of_week(int year, int month, int day) {

    int dow;
    dow = (day \
    + ((153 * (month+12*((14-month)/12)-3)+2)/5)    \
    + (365*(year+4800-((14-month)/12)))             \
    + ((year+4800-((14-month)/12))/  4)             \
    - ((year+4800-((14-month)/12))/100)             \
    + ((year+4800-((14-month)/12))/400)             \
    - 32045
    ) % 7;
    return dow;
}


/*
** Check if provided date falls into the range
** of the central european daylight savings time.
*/
int is_central_europe_dst(int year, int month, int day) {

    if (month < 3 || month > 10) {
        return 0;
    }

    if (month > 3 && month < 10) {
        return 1;
    }

    int ps = day - day_of_week(year, month, day);
    if (month == 3) {
        return (int)(ps >= 25);
    }

    if (month == 10) {
        return (int)(ps < 25);
    }

    return false;
}


/*
** Gets the time of the provided day and location
** when the sun rises.
*/
struct tm get_sunrise(
    int year, int month, int day, 
    float lat, float lng, 
    int offset, 
    int dst) 
{
    float localtime = fmod(24+calc_sun_time(year, month, day, lat, lng, offset, dst, -1), 24);
    double hours;
    float minutes = modf(localtime, &hours)*60;
    struct tm sunrise = {
        .tm_year=year-1900,
        .tm_mday = day,
        .tm_mon = month-1,
        .tm_min = minutes,
        .tm_hour = hours,
        .tm_isdst = dst
    };
    return sunrise;
}


/*
** Gets the time of the provided day and location
** when the sun sets.
*/
struct tm get_sunset(
    int year, int month, int day, 
    float lat, float lng, 
    int offset, 
    int dst) 
{
    float localtime = fmod(24+calc_sun_time(year, month, day, lat, lng, offset, dst, 0), 24);
    double hours;
    float minutes = modf(localtime, &hours)*60;
    struct tm sunset = {
        .tm_year=year-1900,
        .tm_mday = day,
        .tm_mon = month-1,
        .tm_min = minutes,
        .tm_hour = hours,
        .tm_isdst = dst
    };
    return sunset;
}


/*
** Creates a tm struct for the provided date.
*/
struct tm get_date(int year, int month, int day, int dst) {
    struct tm tmday = {
        .tm_year=year-1900,
        .tm_mday = day,
        .tm_mon = month-1,
        .tm_min = 0,
        .tm_hour = 0,
        .tm_isdst = dst
    };
    return tmday;
}


/*
** Get n-days and store the sunrise and sunset time for these 
** days, for a specific location, within a SQLite database.
*/
int main(void) {
    int from        = 2101; // Start year.
    int until       = 2200; // End year.
    int timezone    = 1;    // Timezone offset.
    float lat       = 51.63145; // Latitude of location.
    float lng       = 9.07999;  // Longitude of location.

    char *fname = "./insert_solar_time.sql";
    char *template = read_file(fname);
    
    int y;
    for (y=from; y<=until; y++) {
        int m;
        for (m=FIRSTMONTH; m<=LASTMONTH; m++) {
            int days = days_in_month(y, m);
            int d;          
            for (d=1; d<=days; d++) {
                bool ds = is_central_europe_dst(y, m, d);
                struct tm current = get_date(y, m, d, (int)ds);
                struct tm sunrise = get_sunrise(y, m, d, lat, lng, timezone, (int)ds);
                struct tm sunset = get_sunset(y, m, d, lat, lng, timezone, (int)ds);
                insert_entry(template, &current, &sunrise, &sunset, lat, lng, timezone, (int)ds);
            }
        }
    }
}
```

Answer 1

General Observations

Does my code meet C99 standards?

Not exactly, it meets an earlier standard than C99, since C99 is generally backwards compatible with the earlier standards it compiles just fine in C99.

An example of features in C99 that aren't used is that for loop control variables can be declared in the for statement, so that

static int callback(void *ignore, int argc, char **argv, char **azColName) {
    int i;
    for (i=0; i<argc; i++) {
        printf("%s = %s\n", azColName[i], argv[i] ? argv[1] : "NULL");
    }
    printf("\n");
    return 0;
}

can be written this way:

static int callback(void* ignore, int argc, char** argv, char** azColName) {
    for (int i = 0; i < argc; i++) {
        printf("%s = %s\n", azColName[i], argv[i] ? argv[1] : "NULL");
    }
    printf("\n");
    return 0;
}

The code is generally portable.

Rather than hard coding the name of the database in the execute_command() function, the code would be more useful if the program parameters accepted the name of the database as well as the name of the input file.

Program Organization

The code is complex enough that you could break it up into multiple source files with header files. This would allow the main program to run with different database solutions by using different header and source files. It would also make the code easier to maintain.

Since the code is currently all in one file, all of the functions could be declared as static functions; that won't be true if the code gets broken up.

Remove Code That Isn't Used

The following symbolic constants are not used in the program:

// FILE constants for opening and reading files.
#define FILE_OK 0
#define FILE_NOT_EXIST 1
#define FILE_TOO_LARGE 2
#define FILE_READ_ERROR 3

It would be better to remove them from the code.

Declare the Variables as Needed

In the original version of C back in the 1970s and 1980s variables had to be declared at the top of the function. That is no longer the case, and a recommended programming practice to declare the variable as needed. In C the language doesn't provide a default initialization of the variable so variables should be initialized as part of the declaration. For readability and maintainability each variable should be declared and initialized on its own line. This applies to the for loop I mentioned above as well as in other functions.

In the following function the variable length can be declared within if (fp) {

/*
** Read the files contents and return them.
*/
char* read_file(const char* fname) {

    char* buffer = 0;
    long length;

    FILE* fp = fopen(fname, "rb");

    if (fp) {
        fseek(fp, 0, SEEK_END);
        length = ftell(fp);
        fseek(fp, 0, SEEK_SET);
        buffer = (char*)malloc((length + 1) * sizeof(char));
        if (buffer) {
            fread(buffer, sizeof(char), length, fp);
        }
        fclose(fp);
    }
    buffer[length] = '\0';
    return buffer;
}

In the above function the statement buffer[length] = '\0'; has undefined behavior if the allocation of buffer failed, so that assignment should be within the if (buffer) { statement. If the code used calloc() rather than malloc(), that assignment is unnecessary since buffer would be initialized to all zeros.

There is no need to cast the result of malloc() to char *, since malloc returns void *. In the older version of C it was only necessary to cast the return from malloc to another type if the type wasn't char * since the older versions of malloc returned char *.

Convention When Using Memory Allocation in C

When using malloc(), calloc() or realloc() in C a common convention is to use sizeof *PTR rather sizeof (PTR_TYPE), as this make the code easier to maintain and less error prone, since less editing is required if the type of the pointer changes.

Variable Names

The function str_replace() has some variable names that need to be clarified: the variable name src is pretty common, but trg and with are not so clear, I can guess that trg is target, but it isn't really clear. It really isn't clear what the variable ins is.

The declaration for retVal isn't necessary until the call to malloc() in the str_replace() function. The variable ins is only necessary in the while loop and so can be declared within the loop.

Answer 2

Overall

• Ambitious and good effort.

• Enable all compiler warnings.

• Use double instead of float.

• Make use of C standard library time functions more.

• Add error checking.

• Document algorithms - perhaps by URL reference.

---

Does my code meet C99 standards?

Not quite.

Missing return

days_in_month() fails to return a value if month outside 1-12 range. That is OK in OP's case: m = FIRSTMONTH; m <= LASTMONTH; m++, but undefined behavior in general use when the return value is used.

Lots of questionable code

Enable all compiler warnings. When I compiled (with gcc) and used "pedantic -Wall -Wextra -Wconversion -fmessage-length=0", about 40+ warnings occurred. Many of these are ignorable, but that is only after analysis that should have been done on OP's part. Then the code should have been amended to quiet the warning.

Rather than have me do the compiler's job, better for OP to enable all warnings and address them. One's that are unclear how to addressed could then be asked about.

---

General review

Better to use float constants for float objects

Sometimes using a double constant for a float object makes a difference.

// float lat = 51.63145;
float lat = 51.63145f;

Little need for an imprecise pi

Such famous constants deserved higher precision, even if the code, as compiled today, does not need it. I recommend about 2x the anticipated need.

// #define PI 3.1415926
#define PI 3.1415926535897932384626433832795

Lack of I/O checking

Robust code will check return values of I/O functions like fseek(), ftell(), fread().

Unsafe code

buffer[length] = '\0'; is undefined behavior (UB) should fopen() or malloc() fail.

Pedantic: string length

String lengths may exceed INT_MAX. Use size_t to handle all string sizes. This alone accounts for many compiler warnings.

Improve error messages

When printing a questionable string in an error message, consider adding sentinels about the string. Keep in mind the error occurred for some reason and the string may contain unclear text (i.e. leading/trailing space).

// fprintf(stderr, "Error: %s\n", Msg);
fprintf(stderr, "Error: \"%s\"\n", Msg);

Avoid repeating magic numbers

Use derived values.

char datebuff[17];
// strftime(datebuff, 17, "%Y-%m-%d", date);
strftime(datebuff, sizeof datebuff, "%Y-%m-%d", date);

Avoid naked magic numbers

Why 17? Document such constants.

// State reason for 17 here. 
#define DATEBUFF_SZ 17

char datebuff[DATEBUFF_SZ];
strftime(datebuff, sizeof datebuff, "%Y-%m-%d", date);

As code does not check the return value of strftime(datebuff, 17, "%Y-%m-%d", date); to see if the buffer was insufficient, and since this is only a matter of a few bytes, be safe and allocate generously.

// Room for 3 `int`, format and \0
#define DATEBUFF_SZ (3*11 + 2  + 1)

Pedantic: add checks

//int len = snprintf(NULL, 0, "%f", lat);
//char * fbuff = malloc(len+1);

int len = snprintf(NULL, 0, "%f", lat);
if (len < 0) TBD_Error(); 
char * fbuff = malloc(len+1);
if (fbuff == NULL) TBD_Error(); 

Alternative to malloc()/free()

With C99 and optionally with later C, use a variable length array.

int len = snprintf(NULL, 0, "%f", lat);
// char * fbuff = malloc(len+1);
char fbuff[len + 1];
snprintf(fbuff, len+1, "%f", lat);
template = str_replace(template, "{3}", fbuff);
// free(fbuff);

Advanced: Yet there is another way that used a fixed width buffer. "%f" will only be so long. Such calculations are doable, yet once must be careful. For learners, consider a 2x buffer

//                  -   digits           .   fraction \0
#define FMT_F_SIZE (1 + FLT_MAX_10_EXP + 1 + 6       + 1)
char fbuff[FMT_F_SIZE * 2];
int len = snprintf(fbuff, sizeof fbuff, "%f", lat);

With OP's code in insert_entry(), consider a single large buffer rather than a series of allocated/free'd ones.

Naked algorithms

Calculate day of the year and others deserve a reference or comments as the algorithm is not obvious. Much about this deserves integer math rather than float math.

Mixed float and double math

0.9856 is double and so code upconverts t, performs a double *, then a double - and then down converts to float. Using same type constants is called for.

float M = (0.9856*t)-3.289;
float M = (0.9856f*t)-3.289f;

double functions with float objects

Use float functions.

// float L = fmod(M+(1.916*sin((PI/180)*M)) + (0.020 * sin(2*(PI/180)*M)) + 282.634, 360.0);
float L = fmodf(M+(1.916f*sinf((PI/180)*M)) + (0.020f * sinf(2*(PI/180)*M)) + 282.634f, 360.0f);

Why even bother with all the float?

In C, double is the default type for constants, printing, etc.

Unless code has a compelling reason for float, use double throughout.

Advance: degree trig functions

Code is often converting degree arguments into radians and then calling trig functions.

Instead, form a helper function double sind(double degrees) and call that.

What is especially useful in that astronomical calculations often involve angles far outside the [-360 ... +360] range and range reductions can be done exactly in degrees. This reduces error accumulations.

Careful with edges

With acos(cosH), cosH is a computed value that might just be a wee amount outside [-1.0...+1.0]. Since that is a computation artifact, be prepared for it.

if (cosH > 1.0) cosH == 1.0;
else if (cosH < -1.0) cosH == -1.0;
...

Year range

As OP has code like year+4800, it hints that code is expected to work with a wide year range. If that is the case, a whole slew of other issues come in.

days_in_month(), day_of_week() miscalculates for early year. See Gregorian Calendar.

If the usable year range is much smaller, consider using mktime() to find the day-of-the-week and day-of-the-year.

This issue also applies is to is_central_europe_dst() as the algorithm used there is not valid outside a select year range.

State conventions

Example: is + longitude east or west? I assume east, yet it is good to be clear.

Cast not needed

bool ds = ...
//struct tm current = get_date(y, m, d, (int)ds);
struct tm current = get_date(y, m, d, ds);

Answer 3

Let's look closely at calc_sun_time(). The first thing that strikes me is that it returns a float, but there's nothing to indicate how that relates to a physical quantity. We really need a comment here.

Attempting to track this down by inspecting the call site reveals that the return value is the number of hours into the day. But that's inconsistent with using an integer type for localOffset if that's intended to be a time-zone offset, because then the program will be unusable in regions with offsets that aren't an exact number of hours.

I'm confused as to why we even need to pass localOffset and daylightSavings (and why isn't the latter a bool?) Given that we include <time.h>, there's no reason to implement our own calendrical calculations by hand. Why not to use the facilities available? For example, if we compute the Julian day by subtracting the 1st of January and dividing by 86400, it's more obviously correct than this computation:

float N1 = floor(275*month/9);
float N2 = floor((month+9)/12);
float N3 = (1+floor((year-4*floor(year/4)+2)/3));
float N = N1-(N2*N3)+day-30;

I don't see any code that accounts for the days that have no sunrise or sunset, and the resultant NaN values are likely to cause serious errors for locations within the polar regions.

Returning values before 0:00 or after 24:00 looks likely to produce erroneous output as we make no attempt to normalise such times; again this is most likely in polar regions. It would be good practice to make sure all such times are normalised, probably by using time_t internally, and then converting to struct tm using localtime() for display. In this case, we shouldn't ever be converting to local, since we're not displaying: we should be storing times into database as UTC, and only converting when we retrieve and present them.

Answer 4

(This answers tries to address good practices, but doesn't consider the question of "standard c". This answer is also specific to POSIX)

Standard constants

Use M_PI from math.h, or define PI as M_PI.

Comments

You could explain what the value -.83 represents. Is it an angle? If so, is it in radians or degrees, and what does it measure? Maybe include a Wikipedia link.

Listen to compiler warnings

These are from clang 15. See for yourself on godbolt. These are the ones I'd consider important:

<source>:39:9: warning: variable 'length' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
    if (fp) {

Very important.

<source>:70:15: warning: implicit conversion loses integer precision: 'unsigned long' to 'int' [-Wshorten-64-to-32]
    len_rep = strlen(trg);

Follow the return type of strlen and use size_t for pointer-sized things.

<source>:155:29: warning: cast from 'const char *' to 'char *' drops const qualifier [-Wcast-qual]
    char *template = (char*)insert;

Declare insert as char *, not const char *, so the signature doesn't tell lies.

<source>:212:16: warning: implicit conversion loses floating-point precision: 'double' to 'float' [-Wimplicit-float-conversion]
    float N1 = floor(275*month/9);
<source>:218:24: warning: implicit conversion loses floating-point precision: 'double' to 'float' [-Wimplicit-float-conversion]
    float lngHour = lng/15.0;

Be consistent and use either float or double consistently. Use floorf and 15.0f for floats.

<source>:296:1: warning: non-void function does not return a value in all control paths [-Wreturn-type]
}

This is about days_in_month. Think about what happens if the argument is 42.