Calculate min, max, mean and median out of array Version 2

Question

This question is a follow up of: Calculate min, max, mean and median out of array

I took all the suggestions an reworked the code. Like the description of the excercise suggests i only use one function for all the computations. Also with the sorting the max and median values can now be calculated easier.

I wonder if its good to return the Macro types INT_MIN INT_MAX and NAN for indicating no valid result?

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <assert.h>
#include <limits.h>
#include <math.h>

struct Summary_data {
    long long smallest;
    long long largest;
    double median;
    double mean;
};

static inline int cmp(void const *lhs, void const *rhs) {
    const int *left = (const int *)lhs; 
    const int *right = (const int *)rhs;
    return (*left > *right) - (*left < *right);
}

int* intdup(const int * source, const size_t len)
{
    assert(source);

    int * p = malloc(len * sizeof(*source));

    if (p == NULL)
        exit(1);

    memcpy(p, source, len * sizeof(*source));
    return p;
}

struct Summary_data calculate_values(const int* array, const size_t len)
{
    assert(array);

    struct Summary_data summary_data = { 0 };
    int* calc_array = intdup(array, len);
    size_t i;

    if (len <= 0) {             // dont bother to calc if invalid len
        free(calc_array);
        calc_array = NULL;
        summary_data.smallest = INT_MAX;
        summary_data.largest = INT_MIN;
        summary_data.mean = NAN;
        summary_data.median = NAN;
        return summary_data;
    }

    qsort(calc_array, len, sizeof *array, cmp);

    summary_data.smallest = calc_array[0];
    summary_data.largest = calc_array[len - 1];

    for (i = 0; i < len; ++i) {
        summary_data.mean += calc_array[i];
    }
    summary_data.mean /= len;

    if (len % 2 == 0) { // is even == return the arithmetic middle of the two middle values
        summary_data.median = (calc_array[(len - 1) / 2] + calc_array[len / 2]) / 2.0;
    }
    else {  // is odd  == retunr the middle
        summary_data.median = calc_array[len / 2];
    }

    free(calc_array);
    calc_array = NULL;

    return summary_data;
}


void print_result(const struct Summary_data* summary_data)
{
    assert(summary_data);

    printf("smallest: %i\n", summary_data->smallest);
    printf("largest: %i\n", summary_data->largest);
    printf("median: %g\n", summary_data->median);
    printf("mean: %g\n\n", summary_data->mean);
}

int main()
{
    int test_array[] = { 1,7,3,4,5,6,7,8,9 };       // 9 elements  
    //  int test_array[] = { 1,7,3,4,5,6,7,8 };         // 8 elements  

    int len = sizeof(test_array) / sizeof(test_array[0]);

    //len = 0;                                          // test when len is invalid
    struct Summary_data summary_data = calculate_values(test_array, len);

    print_result(&summary_data);
    getchar();
    return 0;
}

Answer 1

BUGS

1. when calculate_values() is called with a len of 0, the program may exit. this is because calc_array is created before the len <= 0 check, and malloc(0) is allowed by the standard to return 0. To fix this, either allocate calc_array after the len <= 0 check, or change how intdup() handles NULL pointers.

nitpicky stuff

1. The function intdup() exits on a malloc error. In a function as low level as this, it might be a better idea to just return the NULL pointer and let other parts of your program decide what to do. this makes the function reuseable.

2. The function intdup() does nothing that is specific to integers, a function void *dup(const void *source, size_t len) would be more useful.

3. INT_MAX and INT_MIN are gennerally not good for signaling errors, because they are both valid values. In this case however, you can detect invalid values when smallest > largest.

4. The function name calculate_values() is not very descriptive. Something like summarize_array() would better express what the function does.

5. When printing the summary data, you can simplify the multiple printfs into one:

   printf("smallest: %i\nlargest: %i\nmedian: %g\nmean: %g\n\n",
       summary_data->smallest, summary_data->largest,
       summary_data->median, summary_data->mean);
   

6. Instead of declaring variables like i at the beginning of your function, decalre them inline c99 style, or right before the for loop.

Answer 2

To add to Peter's answer:

I can only think of one case where you couldn't compute a valid result (min, median, max, mean) for an array and that's when the array is empty (or infinite I suppose, but if that's a problem for you please let me know where you buy your RAM). Since this is trivial to check I would generally leave it up to the caller to do so (and document that expectation in the function's interface). It's just simpler and cleaner.

I have some minor style suggestions:

• I advise against using assertions for error checking (except for during debugging). Assertion failures cause your program to immediately terminate. In general you would probably want to propagate errors back up the call stack so that the caller can handle them (hopefully) gracefully.

• You check if len is less than zero, but this can't occur as len has an unsigned type.

• It makes sense to check len before allocating calc_array. In general, if you are checking parameters you generally want to do it at the beginning of a function. It's more readable and may save you work.

• I generally prefer to use the types defined in stdint.h when reasonable (uint32_t, uint64_t etc.).

Answer 3

The posted code causes the compiler to output several warning messages.

When compiling, always enable the warnings, then fix those warnings.

EDIT: (added)

Per the OPs request to list the warnings:

 compiled with:
 gcc -ggdb  -Wall -Wextra  -Wconversion -std=gnu11 -pedantic -Wmissing-prototypes  -c "untitled2.c"   -I. (in directory: /home/rkwill/Documents/forum)

 untitled2.c:21:6: warning: no previous prototype for ‘intdup’ [-Wmissing-prototypes]
 int* intdup(const int * source, const size_t len)
      ^

 untitled2.c:34:21: warning: no previous prototype for ‘calculate_values’ [-Wmissing-prototypes]
 struct Summary_data calculate_values(const int* array, const size_t len)
                     ^

 untitled2.c: In function ‘calculate_values’:
 untitled2.c:60:23: warning: conversion to ‘double’ from ‘size_t {aka const long unsigned int}’ may alter its value [-Wconversion]
 summary_data.mean /= len;
                      ^

 untitled2.c: At top level:

 untitled2.c:76:6: warning: no previous prototype for ‘print_result’ [-Wmissing-prototypes]
 void print_result(const struct Summary_data* summary_data)
      ^

 untitled2.c: In function ‘print_result’:
 untitled2.c:80:12: warning: format ‘%i’ expects argument of type ‘int’, but argument 2 has type ‘long long int’ [-Wformat=]
 printf("smallest: %i\n", summary_data->smallest);
                          ^

 untitled2.c:81:12: warning: format ‘%i’ expects argument of type ‘int’, but argument 2 has type ‘long long int’ [-Wformat=]
 printf("largest: %i\n", summary_data->largest);
                         ^

 untitled2.c: In function ‘main’:
 untitled2.c:94:69: warning: conversion to ‘size_t {aka const long unsigned int}’ from ‘int’ may change the sign of the result [-Wsign-conversion]
 struct Summary_data summary_data = calculate_values(test_array, len);
                                                                 ^

Note: When the body of a function is listed before the function is ever invoked, then you can ignore the warnings about the missing prototypes

Answer 4

Stay with the same object

// qsort(calc_array, len, sizeof *array, cmp);
qsort(calc_array, len, sizeof *calc_array, cmp);

Expect min/max to use same type as array

struct Summary_data {
  // long long smallest;
  // long long largest;
  int smallest;
  int largest;

Sum integers with integers

The mean calculation first sums all the array elements with summary_data.mean += calc_array[i];. This is OK, but may lose precision if int is wide or len is great. Mixing floating point (FP) and integer types should be done with care. Here I would consider conditional code.

// Is intmax_t significantly wider than int?
#if INTMAX_MAX/INT_MAX > INT_MAX
  intmax_t sum = 0;
#else
  double sum = 0.0;
#endif
for (i = 0; i < len; ++i) {
    sum += calc_array[i];
}
summary_data.mean = 1.0*sum/len;

malloc returning NULL is not always an error

With a requested size of 0, malloc() may return NULL. Adjust test.

int * p = malloc(len * sizeof(*source));
// if (p == NULL)
if (p == NULL && len > 0)
    exit(1);

Initialize with default value

// struct Summary_data summary_data = { 0 };
struct Summary_data summary_data = { .smallest = INT_MAX,  .largest = INT_MIN, 
    .median = NAN, .mean = NAN};

Now if (len <= 0) { is easy

if (len <= 0) {
    free(calc_array);
    return summary_data;
}

Allocate after len <= 0 test

if (len <= 0) {
    // free(calc_array);
    return summary_data;
}
int* calc_array = intdup(array, len);

Use matching specifiers/types

// printf("smallest: %i\n", summary_data->smallest);
printf("smallest: %lli\n", summary_data->smallest);

Greater vs Largest vs Maximum

Using .largest seems off. Consider which is "larger" -1000 or 2 leaves some ambiguity. I'd use .max or .maximum or maybe even .greatest, but not .largest.

Pedantic overflow avoidance

(calc_array[(len - 1) / 2] + calc_array[len / 2]) may overflow.

// summary_data.median = (calc_array[(len - 1) / 2] + calc_array[len / 2]) / 2.0;
summary_data.median = (1.0*calc_array[(len - 1) / 2] + calc_array[len / 2]) / 2.0;
// OR
int m1 = calc_array[(len - 1) / 2];
int m2 = calc_array[len / 2];
summary_data.median = m1/2 + m2/2 + (m1%2 + m2%2)/2.0; 

This last issues in an interesting corner of coding: How to average two integers with respect to a potentially wide integer and double.