Skip to main content
fixed bug in example code.
Source Link
pacmaninbw
  • 24.8k
  • 11
  • 46
  • 109
static int get_integer_arg(char* argv, char* usage)
{
    int intVal;intVal = 0;
    if (sscanf(argv[3]argv, "%i", &intVal) != 1) {
        fprintf(stderr, "Expected an integer for the %s.\n", usage);
    }

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

    return intVal;
}
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;
}
Completed the review.
Source Link
pacmaninbw
  • 24.8k
  • 11
  • 46
  • 109

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.

I've run outThe functions ftell() and fseek() predate the addition of time atsize_t to the momentC programming standard, I may be ablethey have to add more laterremain 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.

    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;
    if (sscanf(argv[3], "%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;
    }

I've run out of time at the moment, I may be able to add more later.

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

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.

    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;
    if (sscanf(argv[3], "%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;
    }
Fixed typo in example code. Mentioned by chux.
Source Link
pacmaninbw
  • 24.8k
  • 11
  • 46
  • 109
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;
    }
}
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()");
        return 0;
    }
}
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;
    }
}
Source Link
pacmaninbw
  • 24.8k
  • 11
  • 46
  • 109
Loading