Presentation
Code formatted and presented well. Refreshing.
Initialization
With median_init()
, the queue is pre-populated with the value of 0. This implies that the return values of min,median,max
are all bogus until n
values added. it would make sense to either
Allow an initialization value: median_init(...., float init_value)
Or better, functions on min,median,max
as if it had less than n
elements are in it until n
elements are added. In the case of 0 elements, min,median,max
could return NAN
Complexity
Instead of the structures involved, this task could be solved with less data usage: a length and a single array as below and still maintain O(n) complexity.
typedef struct MedfiltDataAlt {
float value;
size_t queue_order;
} MedfiltDataAlt;
Allocation responsibility
This code obliges the calling code to provide the space needed. This fine. Yet I find it more useful for the median_...()
functions to handle that. OP's choice on this, just wanted to provide an alternative POV.
Added functionality
A means to report the min,median,max
without adding a new number would be useful.
Parameter order
I'd expect the state data first.
// void median(float input, MedfiltData *data, float *median, float *min, float *max)
void median(MedfiltData *data, float input, float *median, float *min, float *max)
NAN
As float
can take on a value of not-a-number. Code's use of >
and <
does not clearly push the NAN
is a proscribed manner. When NANs are involved, >
is not the opposite of <=
. If code was amended to deal with a growing array during the first n
calls of median()
, consider a median(NAN, ...)
to have the effect of shortening the queue and not saving the NAN.
Naming
The .c file (assume medfilt.c
to match medfilt.h
) name, the type names and function names would be more uniform if they all began with the same prefix and case rather than Medfilt
, median
and medfilt
.
This is manifested in the minor code error in main.c
// medfilt_init(&data, nodes, KERNEL);
median_init(&data, nodes, KERNEL);
Portability
#pragma once
is not defined defined in the C standard
Function at 0
Minor: Both median()
and median_init()
fail with a size of 0. I would expect code to tolerate this edge case and not cause undefined behavior.
display()
Minor: I'd expect code to use "%zu"
to match size_t
or a cast to a wide unsigned type to maintain same sign-ness. Also int min, int mid, int max
--> float min, float mid, float max
and use "%g"
or "%e"
. A goal of display
is diagnostic. It should avoid data truncation.
// printf("u[%d] =...", (int)i, ...);
printf("u[%zu] =...", i, ...);
printf("u[%lu] =...", (unsigned long)i, ...);