Electrocardiography simulator

I am tring to learn coding in clear way. I developed easy application to simulate EKG. I would like to ask some experts, if the code is clean and if they have any suggestion, what could I improve. Possible improvment is not to avoid first loop (no counter of data is set to -1, to avoid first read), but I do not have an idea how to make it.

EkgDataModel

struct ekgDataModel {
    int amountOfData;
    int frequency;
    float newValue;
    float previousValue;
};

void initEKGData(struct ekgDataModel *ekgData) {
    // -1 to avoid first loop
    ekgData->amountOfData = -1;
    ekgData->frequency = 0;
    ekgData->newValue = 0;
    ekgData->previousValue = 0;
}

EkgFunctionality.c

void runElectrocardiography() {
    struct ekgDataModel ekgData;
    initEKGData(&ekgData);

    while (ekgData.newValue != END_VALUE) {
        analyzeNewValue(&ekgData);
        ekgData.previousValue = ekgData.newValue;
        ekgData.newValue = getNewInput();
    }
}

void analyzeNewValue(struct ekgDataModel* ekgData) {
    if (ekgData->newValue >= MIN_INPUT_RANGE && ekgData->newValue amountOfData);
        ekgData->frequency += checkCrossing(ekgData);
        checkIfFrequencyValid(ekgData);
    }
}

void checkIfFrequencyValid(struct ekgDataModel* ekgData) {
    if (ekgData->amountOfData == MAX_AMOUNT_OF_DATA
        && (ekgData->frequency frequency > MAX_FREQUENCY_ALLOWED)) {
        displayWrongFrequencyMessage(&ekgData->frequency);
        ekgData->frequency = 0;
        ekgData->amountOfData = 0;
    }
}

int checkCrossing(struct ekgDataModel* ekgData) {
    if (ekgData->newValue * ekgData->previousValue < 0)
        return 1;
    else
        return 0;
}

EkgInputOutputOperations.c

float getNewInput() {
    float newValue;
    scanf("%f", &newValue);
    return newValue;
}

void displayWrongFrequencyMessage(int *frequency) {
    printf("Bad frequency result: %d\n", *frequency);
}

I see a number of things that might help you improve your code.

Arrange functions from simple to compound

Within your source code files, try to arrange your functions such that simpler functions, such as checkCrossing precede compound functions such as analyzeNewValue that call other simple functions. Doing so allows you to avoid having to write function declarations that would otherwise be required.

Use const where practical

The checkCrossing function is passed a pointer to an ekgDataModel and the function does not alter that structure. This should be made explicit by declaring the parameter const as in:

int checkCrossing(const struct ekgDataModel* ekgData) {

Carefully consider your data

The checkCrossing routine is looking for zero-crossings and uses this expression:

if (ekgData->newValue * ekgData->previousValue < 0) { /* ... */ }

However, consider the possibility that one of the samples is exactly 0. For example, a sequence might be -0.15, 0.0, +0.20. For (-0.15, 0.0) the expression is false and for (0.0, +0.20) the expression is again false. The effect is that the function has just missed an obvious zero-crossing. An alternative approach would be to use the signbit macro:

if (signbit(ekgData->newValue) != signbit(ekgData->previousValue)) { /* ... */ }

I'd rewrite the function using stdbool.h as a bool function:

bool isZeroCrossing(float prev, float curr) {
    return (bool)signbit(curr) != (bool)signbit(prev);
}

Prefer >= to == to prevent "runaway data"

In the checkIfFrequencyValid function, the program currently does this:

if (ekgData->amountOfData == MAX_AMOUNT_OF_DATA
    && (ekgData->frequency frequency > MAX_FREQUENCY_ALLOWED)) {
     /* stuff */
     ekgData->amountOfData = 0;
}

There is a potential problem here in that the analyzeNewValue function only increments amountOfData, so if it ever gets to MAX_AMOUNT_OF_DATA and the frequency is within range, subsequent readings will increase amountOfData beyond MAX_AMOUNT_OF_DATA which would make for a difficult-to-debug error. Instead, use the >= operator for the first clause to make sure that you don't have "runaway data" problems.

Use better names

Function names such as displayWrongFrequencyMessage and initEKGData are good because they give a very good idea of what the functions are doing just by the name. However functions like checkCrossing and checkIfFrequencyValid are not so good. In one case, essentially a boolean value is returned indicating that a zero crossing has occurred, but in the other, nothing is returned at all. Better for boolean functions is to name them to suggest what the return value might mean. For example isZeroCrossing would return true if it detected a zero crossing. Very clear and doesn't require much further explanation.

Check return values of standard functions

Many standard functions, including scanf can fail and return values that can help in detecting those failures. To write robust software, you should get into the habit of checking the return values and dealing intelligently with the potential failures.

Avoid scanf if you can

There are so many well known problems with scanf that you're usually better off avoiding it. The usual approach is to read in user input into a string and then parse from there, in this case using something like atof().

Think carefully about signed versus unsigned

Would it ever be possible that amountOfData or frequency would be negative numbers? If so, what would that mean? Better would be to declare them both as unsigned so that all possible values have an interpretable meaning.

Prefer pass-by-value for simple data

Generally, unless there is a need to modify the underlying variable, it's better to pass a value rather than a pointer. For instance, this function:

void displayWrongFrequencyMessage(int *frequency) {

Could instead be written like this:

void displayWrongFrequencyMessage(int frequency) {

That way there's no way that the function could alter the original frequency variable and it's likely to be efficiently passed in a register.

Write code for humans to read it

The runElectrocardiography function is not bad, but it's not easy to understand. I'd recommend doing something like this instead:

```
void runElectrocardiography() {
unsigned dataCount = 0; // number of data points so far
unsigned crossingCount = 0; // number of zero-crossings
float curr; // the most recent EKG voltage value
float prev = 0; // the previous EKG voltage value

for (curr = getNewValue(); curr != END_VALUE; curr = getNewValue()) {
if (isWithinValidRange(curr)) {
++dataCount;
if (isZeroCrossing(prev, curr)) {
++crossingCount;
}
if (!isFrequencyValid(dataCount, crossingCount)) {
displayWrongFrequencyMessage(crossingCount);

StackExchange Code Review Q#121765, answer score: 3