6
\$\begingroup\$

This is a follow-up to the question: C program for storing the time of the sunset and sunrise
After receiving a lot of great help and critique I've updated my code.

Questions

  1. Are there major issues with my code?
  2. How can I further improve my code?

FOLLOW UP

Compiler Warnings
I mostly concentrated on fixing the compiler warnings and set my compiler options to the following: -lm -lsqlite3 -pedantic -Wall -Wextra -Wconversion -fmessage-length=0.

SQL
I changed the SQL insert behavior to execute them in several transactions for increased performance.

size_t
A thing that I don't quite understand in C is the need for back and forth conversion between int and size_t. More often than not it seems rather unnecessary to me as you can just check and terminate beforehand, it is also quite rare that you get a negative int which would result in a large size_t due to the sign-bit. I also don't quite understand why many functions return signed integers when the output of those functions is most often used for allocation, at least in my observation.

Parameters
Parameters, except for latitude and longitude, are now passed to the main and validated.

What I haven't done

  1. I did not update the code to support regions outside of central Europe as I'm using this code purely for me, privately. This is also the reason why I don't check for times/regions where the sun never sets or rises.
  2. I barely make use the time.h features which I plan on changing soon.
  3. I haven't yet spliced up the code into individual files but I plan on doing this as the next step.

Code
I've uploaded the code and the DDL for creating the database table as well as the insert template to this repo.

Here the updated code:

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

// Defintion for PI.
#define PI 3.1415926535897932384626433832795

// Sun's zenith in degrees.
// For sunrise and sunset calculations a 
// degree of 0.833 is assumed which 
// accounts for atmospheric refraction.
#define ZENITH -.83

// Defined latitude and longitude.
// I only want my own personal position.
#define LATITUDE 50.000000
#define LONGITUDE 11.00000

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


/*
** Calculate the sunrise or sunset for given date
** and location provided by latitude and longitude.
**
** INFO: The longitude is positive for East and negative for West.
** INFO: The trigonemtry functions are using degree instead of radian.
**       Factor is 180/PI.
** INFO: 
*/
static double calc_sun_time(
  int year, int month, int day, 
  double lat, double lng, 
  int localOffset, 
  int daylightSavings, 
  int settingTime) 
{

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

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

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

  // Calculate sun's true longitude.
  double 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.
  double RA = fmod(180/PI*atan(0.91764*tan((PI/180)*L)), 360.0);

  // Right ascension value need to be in the quadrant as L.
  double Lquadrant = floor(L/90)*90;
  double 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.
  double sinDec = 0.39782*sin((PI/180)*L);
  double cosDec = cos(asin(sinDec));

  // Calculate the sun's local hour angle.
  double cosH = (sin((PI/180)*ZENITH)-(sinDec*sin((PI/180)*lat))) / (cosDec*cos((PI/180)*lat));
  
  double H = .0f;
  if (settingTime) {
    H = 360-(180/PI)*acos(cosH);
  } /* rising */
  else {
    H = (180/PI)*acos(cosH);
  } /* setting */
  H = H/15;

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

  // Adjust back to UTC;
  double 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.
*/
static int days_in_month(int year, int month) {

  if (month < 1 || month > 12) {
    return 0;
  }

  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;
  }
  return 0;
}


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

  int 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.
*/
static 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 0;
}


/*
** Gets the time of the provided day and location
** when the sun rises.
*/
static struct tm get_sunrise(
  int year, int month, int day, 
  double lat, double lng, 
  int offset, 
  int dst) 
{
  double localtime = fmod(24+calc_sun_time(year, month, day, lat, lng, offset, dst, -1), 24);
  // Precision of double is required
  // to successfully convert to hours
  // and minutes. 
  // Hours and minutes are explicitly converted
  // to int later on.
  double hours;
  double minutes = modf(localtime, &hours)*60;
  struct tm sunrise = {
    .tm_year= year-1900,
    .tm_mday = day,
    .tm_mon = month-1,
    .tm_min = (int)minutes,
    .tm_hour = (int)hours,
    .tm_isdst = dst
  };
  return sunrise;
}


/*
** Gets the time of the provided day and location
** when the sun sets.
*/
static struct tm get_sunset(
  int year, int month, int day, 
  double lat, double lng, 
  int offset, 
  int dst) 
{
  double localtime = fmod(24+calc_sun_time(year, month, day, lat, lng, offset, dst, 0), 24);
  // Precision of double is required
  // to successfully convert to hours
  // and minutes. 
  // Hours and minutes are explicitly converted
  // to int later on.
  double hours;
  double minutes = modf(localtime, &hours)*60;
  struct tm sunset = {
    .tm_year= year-1900,
    .tm_mday = day,
    .tm_mon = month-1,
    .tm_min = (int)minutes,
    .tm_hour = (int)hours,
    .tm_isdst = dst
  };
  return sunset;
}


/*
** Creates a tm struct for the provided date.
*/
static 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;
}


/*
** Prepare new SQL statement.
*/
static void format_insert(
  sqlite3_stmt *stmt,
  struct tm *date, 
  struct tm *sunrise, 
  struct tm *sunset, 
  double lat, 
  double lng, 
  int localOffset, 
  int daylightSavings) 
{
  // Length of date formatted (yyyy-MM-dd\n);
  const size_t LEN_DATEFORMAT = 11;
  // Length of time formatted (HH:mm\n);
  const size_t LEN_TIMEFORMAT =  6;

  // Write date into statement.
  char date_buff[LEN_DATEFORMAT];
  strftime(date_buff, LEN_DATEFORMAT, "%Y-%m-%d", date);
  sqlite3_bind_text(stmt, 1, date_buff, (int)strlen(date_buff), 0);
  // Write time of sunrise into statement.
  char sunrise_buff[LEN_TIMEFORMAT];
  strftime(sunrise_buff, LEN_TIMEFORMAT, "%H:%M", sunrise);
  sqlite3_bind_text(stmt, 2, sunrise_buff, (int)strlen(sunrise_buff), 0);
  // Write time of sunset into statement.
  char sunset_buff[LEN_TIMEFORMAT];
  strftime(sunset_buff, LEN_TIMEFORMAT, "%H:%M", sunset);
  sqlite3_bind_text(stmt, 3, sunset_buff, (int)strlen(sunset_buff), 0);
  // Write latitute, longitude, utc offset 
  // and dst into statement.
  sqlite3_bind_double(stmt, 4, lat);
  sqlite3_bind_double(stmt, 5, lng);
  sqlite3_bind_int(stmt, 6, localOffset);
  sqlite3_bind_int(stmt, 7, daylightSavings);
}


/*
** Calculates the time of the sunrise and sunset
** for each day of the year and for each year 
** within the range 'from' and 'until'.
**
** NOTE: insert_template must be a 
**       generic SQLite compliant insert command.
** NOTE: code does not yet check/account for dates
**       below the year 1900.
** NOTE: timezone is the local offset to UTC.
** NOTE: longitude and latitude are defined and 
**       need to be adjusted.
*/
static int insert_entries(
  char *db_path, 
  char *insert_template, 
  int from, int until, 
  int timezone) 
{

  sqlite3 *db;
  char *zErrMsg = 0;
  int rc;

  rc = sqlite3_open(db_path, &db);
  if (rc) {
      fprintf(stderr, "Can't open database: %s\n", sqlite3_errmsg(db));
      sqlite3_close(db);
      return (0);
  }

  // Prepare insert for sqlite3_step().
  sqlite3_stmt *stmt;
  rc = sqlite3_prepare(db, insert_template, -1, &stmt, NULL);
  if (SQLITE_OK != rc) {
    fprintf(stderr, "Unable to prepare insert statement.");
    sqlite3_close(db);
    return (0);
  }

  for (int y=from; y<=until; y++) {
    rc = sqlite3_exec(db, "BEGIN TRANSACTION;", NULL, NULL, &zErrMsg);
    if ( rc ) {
      fprintf(stderr, "Unable to start transaction: %s\n", sqlite3_errmsg(db));
      // Free error message memory.
      sqlite3_free(zErrMsg);
      // Finalize and close.
      sqlite3_finalize(stmt);
      sqlite3_close(db);
      return (0);
    }
    for (int m=FIRSTMONTH; m<=LASTMONTH; m++) {
      int days = days_in_month(y, m);
      if ( !days ) {
        fprintf(stderr, "Unable to get days for month %d.\n", m);
        continue;
      }
      for (int d=1; d<=days; d++) {
        int ds = is_central_europe_dst(y, m, d);
        struct tm current = get_date(y, m, d, ds);
        struct tm sunrise = get_sunrise(y, m, d, LATITUDE, LONGITUDE, timezone, ds);
        struct tm sunset = get_sunset(y, m, d, LATITUDE, LONGITUDE, timezone, ds);
        format_insert(stmt, &current, &sunrise, &sunset, LATITUDE, LONGITUDE, timezone, ds);
        int retVal = sqlite3_step(stmt);
        if (SQLITE_DONE != retVal) {
          fprintf(stderr, "Failed to execute insert. %d\n", retVal);
        }        
        sqlite3_reset(stmt);
      }
    }
    rc = sqlite3_exec(db, "COMMIT TRANSACTION;", NULL, NULL, &zErrMsg);
    if ( rc ) {
      fprintf(stderr, "Unable to commit transaction: %s\n", sqlite3_errmsg(db));
      // Free error message memory.
      sqlite3_free(zErrMsg);
      // FInalize and close.
      sqlite3_finalize(stmt);
      sqlite3_close(db);
      return (0);
    }
  }
  sqlite3_finalize(stmt);
  sqlite3_close(db);
  return (1);
}


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

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

  if ( NULL==fp ) {    
    return 0;
  }
  else {
    long len;
    // Go to end of file to get the length
    // of the file using ftell().
    int success = fseek(fp, 0L, SEEK_END);
    if (0 != success) {
      return 0;
    }
    len = ftell(fp);
    if (0 >= len) {
      return 0;
    }
    // Return to start of file.
    fseek(fp, 0L, SEEK_SET);
    // Initialze null-terminated buffer.
    size_t bufLen = (size_t)(len + 1);
    buffer = calloc(bufLen, sizeof(char));
    if (buffer) {
      fread(buffer, sizeof(char), bufLen, fp);
      int err = ferror(fp);
      // Check if read error occured.
      if (err) {
        fprintf(stderr, "Error [%d] while reading file %s in %s\n", err, fname, __func__);
        fclose(fp);
        return 0;
      }
    }    
    fclose(fp);
  }
  return buffer;
}


/*
** Tries to open a SQLite database connection
** with the given database path.
** Returns 0 on error and 1 on success.
*/
static int test_database(char * db_path) {
  sqlite3 *db;
  int rc;

  rc = sqlite3_open_v2(db_path, &db, SQLITE_OPEN_READONLY, NULL);
  if ( rc ) {
    fprintf(stderr, "Can't open database: %s\n", sqlite3_errmsg(db));
    return 0;
  }
  sqlite3_close(db);
  return 1;
}


/*
** Get n-days and store the sunrise and sunset time for these 
** days, for a specific location, within a SQLite database.
*/
int main(int argc, char *argv[]) {

  if (argc < 5) {
    fprintf(stderr, "Insufficient amount of arguments..\n");
    return 0;    
  }

  if (argc > 6) {
    fprintf(stderr, "Too many arguments (required %i-%i)", 5, 6);
    return 0;
  }
  
  char *db_path = argv[1];
  if ( !test_database(db_path) ) {
    fprintf(stderr, "Unable to open or locate SQLite database with path %s\n", db_path);
    return 0;
  }
  
  char *fname = argv[2];
  char *template = read_file(fname);
  if ( !template ) {
    fprintf(stderr, "Unable to load INSERT template from file %s.\n", fname);
    return 0;
  }

  int from;
  if ( sscanf(argv[3], "%i", &from)!=1 ) {
    fprintf(stderr, "Expected an integer for the start date.\n");
  }

  int until;
  if ( sscanf(argv[4], "%i", &until)!=1 ) {
    fprintf(stderr, "Expected an integer for the end date.\n");
  }

  if (0 == from || 0 == until) {
    fprintf(stderr, "Start year or end year is invalid - start: %i end: %i\n", from, until);
    return 0;
  }

  int utc_offset = 1;
  if ( 6==argc ) {
    if ( sscanf(argv[5], "%i", &utc_offset)!=1 ) {
      fprintf(stderr, "Expected an integer for the UTC offset in hours.\n");
      return 0;
    }
  }

  insert_entries(db_path, template, from, until, utc_offset);
}

\$\endgroup\$
6
  • \$\begingroup\$ You should also add -std=c17 to your compiler flags \$\endgroup\$
    – Reinderien
    Jan 19, 2023 at 13:07
  • \$\begingroup\$ @Reinderien In the original question the OP was concerned about being compliant to C99. They may not have access to C17. \$\endgroup\$
    – pacmaninbw
    Jan 19, 2023 at 15:59
  • 1
    \$\begingroup\$ For size_tgive this a read: stackoverflow.com/q/2550774 \$\endgroup\$
    – Harith
    Jan 19, 2023 at 18:29
  • \$\begingroup\$ H+RA-(0.06571*t)-6.622; ---> space bar makes for readability. Use it. \$\endgroup\$
    – Harith
    Jan 19, 2023 at 18:30
  • 1
    \$\begingroup\$ Max, should you consider a 3rd post, perhaps restrict it to focus on a subset of code. Perhaps one of the "spliced up code" and only enough extra code to allow for a test harness. \$\endgroup\$ Jan 19, 2023 at 20:52

2 Answers 2

6
\$\begingroup\$

General Observations

I was hoping you would do a follow up question.

This is definitely an improvement, I can see more issues I should comment on, which was harder to do in the first version.

I understand this program is for you personally, but, when coding some of the things to keep in mind are that you may not always be the person maintaining the code so the code should be as readable as possible to make maintenance for others easier.

size_t
A thing that I don't quite understand in C is the need for back and forth conversion between int and size_t. More often than not it seems rather unnecessary to me as you can just check and terminate beforehand, it is also quite rare that you get a negative int which would result in a large size_t due to the sign-bit. I also don't quite understand why many functions return signed integers when the output of those functions is most often used for allocation, at least in my observation.

The functions ftell() and fseek() predate the addition of size_t to the C programming standard, they have to remain backwards compatible to earlier versions of the language.

Reminder from previous review :

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

Example: from current code

        buffer = calloc(bufLen, sizeof(*buffer));

Note that sizeof(char) has been replaced with sizeof(*buffer). This allows easy modification of the code, the allocation size will change if the type of buffer changes.

Magic Numbers

There are Magic Numbers in the days_in_month(int year, int month) function (1 through 12, and 29, 28 30, 31), it might be better to create symbolic constants for them to make the code more readable and easier to maintain. These numbers may be used in many places and being able to change them by editing only one line makes maintenance easier.

Numeric constants in code are sometimes referred to as Magic Numbers, because there is no obvious meaning for them. There is a discussion of this on stackoverflow.

In the case of the numbers 1 through 12, an enum might be a good solution, for the number of days in a month use numeric constants, while const hasn't completely replaced #define for constants it is more type safe that using macros. The following code is based on including stdbool.h using true and false is preferable to using 1 and 0.

I believe this would be more readable that the current implementation of days_in_month(int year, int month):

typedef enum {
    January = 1,
    February,
    March,
    April,
    May,
    June,
    July,
    August,
    September,
    October,
    November,
    December
} Months;

static const int FEBRUARYLEAPYEAR = 29;
static const int FEBRUARYDAYS = 28;
static const int LONGMONTHS = 31;
static const int SHORTMONTHS = 30;

static bool is_leap_year(int year)
{
    return ((year % 4 == 0)
        && (year % 100 != 0)
        && (year % 400 == 0));
}

static int days_in_month(int year, Months month) {

    switch (month)
    {
    case January:
    case March:
    case May:
    case July:
    case August:
    case October:
    case December:
        return LONGMONTHS;
    case April:
    case June:
    case November:
    case September:
        return SHORTMONTHS;
    case February:
        return is_leap_year(year)? FEBRUARYLEAPYEAR : FEBRUARYDAYS;
    default:
        fprintf(stderr, "Unknown month in days_in_month()\n");
        return 0;
    }
}

*Note: I believe the above code reduces the need for the comment block before days_in_month() because the code is self documenting. This is important because comments need to be maintained with the code. *

The for loop that references this in insert_entries() could be changed in the following manner:

        for (Months m = January; m <= December; m++) {
            int days = days_in_month(y, (Months)m);
            if (!days) {
                fprintf(stderr, "Unable to get days for month %d.\n", m);
                continue;
            }
            for (int d = 1; d <= days; d++) {
                int ds = is_central_europe_dst(y, m, d);
                struct tm current = get_date(y, m, d, ds);
                struct tm sunrise = get_sunrise(y, m, d, LATITUDE, LONGITUDE, timezone, ds);
                struct tm sunset = get_sunset(y, m, d, LATITUDE, LONGITUDE, timezone, ds);
                format_insert(stmt, &current, &sunrise, &sunset, LATITUDE, LONGITUDE, timezone, ds);
                int retVal = sqlite3_step(stmt);
                if (SQLITE_DONE != retVal) {
                    fprintf(stderr, "Failed to execute insert. %d\n", retVal);
                }
                sqlite3_reset(stmt);
            }
        }

Meaningful Variable Names

For the most part this has been addressed since the first version, but, I still see some variable names I wouldn't necessarily understand if I was maintaining the code. I might understand that lat in format_insert() was latitude but I might not understand the lng is longitude. I have noticed that lat and lng are used in multiple functions.

In get_sunset() I would definitely have to guess at what dst is; my guess is daylight savings time. If it is daylight savings time you might want to convert the type to bool if you include stdbool..

What is ps in is_central_europe_dst()? Should is_central_europe_dst() be returning a boolean variable instead?

Complexity

At least one function, insert_entries, is too complex (does too much). The main() function and calc_sun_time() function could also be similified. There are multiple measures for complexity, one is the number of lines of code in a function, another is the level of indentation in a function, a third would be the number of time the code branches in a function (this might be related to indentation).

A general best practice is to limit the lines in a function to what is viewable in a single screen in and IDE or editor. The reason for this is that it is hard to keep track of everything going on in a function if you have to scroll up or down. My screens are limited to 60 lines, I guess that is about average.

There is also a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states:

that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

In the function calc_sun_time() there are 2 if statements that might become functions;

    double lngHour = lng / 15.0;
    double t = 0.0;
    if (settingTime) {
        t = N + ((6 - lngHour) / 24);
    } /* calculate rising time.  */
    else {
        t = N + ((18 - lngHour) / 24);
    } /* calculate setting time. */

One posibility here is to remove the if statement:

    t = N + (settingTime ? ((6 - lngHour) / 24) : ((18 - lngHour) / 24));

and

    double H = .0f;
    if (settingTime) {
        H = 360 - (180 / PI) * acos(cosH);
    } /* rising */
    else {
        H = (180 / PI) * acos(cosH);
    } /* setting */
    H = H / 15;

In the main() function this section of code could be a function that main calls:

    int from;
    if (sscanf(argv[3], "%i", &from) != 1) {
        fprintf(stderr, "Expected an integer for the start date.\n");
    }

    int until;
    if (sscanf(argv[4], "%i", &until) != 1) {
        fprintf(stderr, "Expected an integer for the end date.\n");
    }

    if (0 == from || 0 == until) {
        fprintf(stderr, "Start year or end year is invalid - start: %i end: %i\n", from, until);
        return 0;
    }

DRY Code

There is a programming principle called the Don't Repeat Yourself Principle sometimes referred to as DRY code. If you find yourself repeating the same code mutiple times it is better to encapsulate it in a function. If it is possible to loop through the code that can reduce repetition as well.

In the above section, the code from main() contains repetition, it might be better to turn this code:

    int from;
    if (sscanf(argv[3], "%i", &from) != 1) {
        fprintf(stderr, "Expected an integer for the start date.\n");
    }

into a function:

static int get_integer_arg(char* argv, char* usage)
{
    int intVal = 0;
    if (sscanf(argv, "%i", &intVal) != 1) {
        fprintf(stderr, "Expected an integer for the %s.\n", usage);
    }

    return intVal;
}

With this being the result:

    int from = get_integer_arg(argv[3], "start date");
    int until = get_integer_arg(argv[4], "end date");
    if (0 == from || 0 == until) {
        fprintf(stderr, "Start year or end year is invalid - start: %i end: %i\n", from, until);
        return 0;
    }
\$\endgroup\$
3
  • 2
    \$\begingroup\$ get_integer_arg() needs works. It returns an uninitialized value when error printed. Note int intVal = 0; is a good but insufficient improvement. \$\endgroup\$ Jan 20, 2023 at 0:21
  • \$\begingroup\$ @chux-ReinstateMonica Did you notice that argv was indexed so that there would have been UB? I think I would prefer atoi. It never pays to rush answers or edits. \$\endgroup\$
    – pacmaninbw
    Jan 20, 2023 at 13:11
  • \$\begingroup\$ pacmaninbw, Yes. \$\endgroup\$ Jan 20, 2023 at 16:51
4
\$\begingroup\$
  1. Are there major issues with my code?

Reconsider astronomical calculations and local time functions: Keep them separate. For astronomy, use double with a convenient epoch like J2000 and scale by only one time unit: seconds (or days or years) - no time zone, leap year, leap second, calendar, A.D/B.C concerns. Then create astro_time_to/from_localtime() routines,

Although I have not deeply analyzed, much math code seems to risk edge cases conditions. e.g. times near midnight and anytime floating point code attempts mod math. This task easier with separation advised in above point.

Code still lacks useful error check in various places.

Code fails in many places if int was 16-bit.


  1. How can I further improve my code?

double vs. int math

Integer math makes more sense than floating point here.

//double N1 = floor(275*month/9);
//double N2 = floor((month+9)/12);

int N1 = 275*month/9;
int N2 = (month+9)/12;

Could continue with int math if year is for nearby years. Yet to handle negative years and large year values, one needs to consider overflow and / which truncates in int math.

//double N3 = (1+floor((year-4*floor(year/4)+2)/3));
//double N = N1-(N2*N3)+day-30;
//                                         vvv
double N3 = 1 + floor((year - 4*floor(year/4.0) + 2)/3);
double N = N1 - (N2*N3) + day - 30;

Still naked magic number

What is 0.9856, -3.289? Please describe what those values are or reference/cite some algorithm that used them. This issues applies to many code constants.

double M = (0.9856*t)-3.289; // ??

Named conversion vs. magic numbers

Instead of (PI/180)*M, consider defining D2R(). (Computation order changed also to improve for popular degree values.)

// Convert degrees to radians.
#define D2R(d)  ((d)/180 * PI)

... and a corresponding R2D().

acos() error prevention

Code still not protecting against acos(x) where |x| is just a bit higher than 1.0.

4800??

What is this 4800 in day_of_week()?

If year range is contemporary, consider mktime() to find day_of_week().

calc_sun_time()

I do not trust using the result of calc_sun_time() in fmod(24+calc_sun_time(year, month, day, lat, lng, offset, dst, -1), 24); to perform as hoped.

Why 24+? If that is to deal with negative return values, when when negative, day should be adjusted, and then maybe month, year.

Instead perhaps:

double tod = calc_sun_time(year, month, day, lat, lng, offset, dst, -1);
// Round to the nearest minute
long minute = lround(tod * (24*60));
if (minute < 0) {
  minute += MINUTES_PER_DAY;
  day--;
}
...
  .tm_min = (int)(minute%60),
  .tm_hour = (int)(minute/60),

Range checking

format_insert() fails when year > 9999.

Confusing comment

Why '\n' in comment? Did you mean \0?

// Length of date formatted (yyyy-MM-dd\n);
const size_t LEN_DATEFORMAT = 11;

Minor: L not needed

// int success = fseek(fp, 0L, SEEK_END);
int success = fseek(fp, 0, SEEK_END);

Looks like Yoda speech

// if (0 >= len) {
if (len <= 0) {

IAC, it is the wrong test. Should be

errno = 0;
if (len == -1 && errno) {

Yet code has trouble down the road with other negative values, so might as well bail with if (len < 0) return 0;.

Allocate to the reference object, not the type

Is the type right? To know, one has to search around for buffer definition`.

buffer = calloc(bufLen, sizeof(char));  // Right size?

Instead

buffer = calloc(bufLen, sizeof buffer[0]);  // Right on inspection.

Error check

  fread(buffer, sizeof(char), bufLen, fp);
  // fread still may have failed to read `bufLen` characters without `ferror()` being true.
  int err = ferror(fp);
  if (err) {

Better as

  if (fread(buffer, sizeof(char), bufLen, fp) < bufLen)) {

Return failure on failure

Let calling code know something failed.

  if (argc < 5) {
    fprintf(stderr, "Insufficient amount of arguments..\n");
    // return 0;    
    return 1;    
    // or 
    return EXIT_FAILURE;    
  }

Minor: Error checking improvement

if ( sscanf(argv[3], "%i", &from)!=1 ) { does not catch "123abc" or "123456789012345678901234567890" as errors. Look to strtol() for an alternative.

or

int n = 0;
sscanf(argv[3], "%8d%n", &from, &n);
// Did conversion happen? Extra text? In range?
if (n == 0 || argv[3][0] || from < 0 || from > DATE_MAX) {

Unneeded code

Why if ( 6==argc ) { test? Prior code already range tested argc.

Range checks

Since OP has limit to "central Europe" and "don't check for times/regions where the sun never sets or rises.", consider adding some grass checks on latitude and longitude the error out.

if (latitude >= ARTIC_CIRCLE) error();

Also some range check on valid years.


... why many functions return signed integers when the output of those functions is most often used for allocation ... In the one place OP"s does this, it is weak code. Sadly there is not a robust ways to find file length. Just common hacks that tend to work.

Further

Code only allocates in one place: calloc(bufLen, sizeof(char)) and uses a derived signed value from long ftell().

This is technically bad. Although this approach to find file length is commonly used, it is not defined well. fseek(fp, 0L, SEEK_END) is UB on a binary file.

The return value of ftell() is problematic. For a text stream, its file position indicator contains unspecified information. For huge files exceeding LONG_MAX, the result may be negative. For huge files exceeding ULONG_MAX, well we are out of luck.

When the file size exceed SIZE_MAX, OP's code has trouble. It did not attempt to detect this.

OP did a good thing though, form a helper function to to this task.

To cope with some of these issues:

errno = 0;
long offset = fell(fp);
if ((offset == -1 && errno) || (unsigned long) offset >= SIZE_MAX) error();
size_t sz = (size_t) offset + 1;
\$\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.