# Averaging numbers from a file, obeying Single Responsibility Principle

Given comma_separated.txt file with string:

1,2,3,4,5,6,7,8,9,10,11,12...,n


Calculate the average value.

For example: 1,2,3 should be equal 2 (float) as result.

Here is my solution in C:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int element_counter;
float* all_elements;

element_counter += 1;
}

return element_counter;
}

float* expand_array_stack_for(float* elements) {
}

void append_float(float element) {
all_elements = expand_array_stack_for(all_elements);
}

float* get_collection() {
return all_elements;
}

FILE *file_opened;

file_opened = fopen(filename, "r");
char elements;
char * element;

while (fgets(elements, sizeof(elements), file_opened) != NULL) {
element = strtok(elements, ",");

while (element != NULL) {
append_float(strtof(element, NULL));
element = strtok(NULL, ",");
}
}

free(element);
fclose(file_opened);

return get_collection();
}

int main() {
float sum = 0.0, average;
int i = 0;

for (i = 0; i < get_number_of_read_elements(); i++) {
sum += elements[i];
}

printf("average = %f\n", average);

return 0;
}


This is my very first time coding in C. I used to code in Ruby, Java and PHP, so maybe this code would be a little bit "high leveled".

As syb0rg mentioned, strtok() is deprecated. Before jumping to replace it with strtok_r() or strtok_s(), though, consider alternatives such as strsep(), strtod() and the scanf() family of functions.

You shouldn't call free(element), since strtok() is returning a pointer to a static buffer.

Memory management in C is a hassle. In this problem, memory management can be an issue in two ways:

• Whenever you read from a file, you have to provide a buffer. You used fgets() to fill a 255-byte buffer named elements. What if the file size exceeds 255 bytes, and a number straddles the 255-byte boundary? Handling that boundary correctly takes a lot of effort!
• Storing a list of numbers of unbounded size is also problematic. You handle that by calling realloc() before appending each number to the list. While that is correct, it's inefficient, as each call to append_float() could involve copying the entire array each time.

Therefore, C programmers prefer solutions that minimize the memory-management headaches.

### A natural C solution

Fortunately, such a solution exists. I would consider this a natural way to solve the problem in C.

#include <stdio.h>

int main(void) {
const char *filename = "comma_separated.txt";
FILE *f = fopen(filename, "r");
if (!f) {
perror(filename);
return 1;
}

double d;
double sum = 0.0;
int count;
while (EOF != fscanf(f, "%lf,", &d)) {
sum += d;
count++;
}
printf("average = %lf\n", sum / count);
fclose(f);
return 0;
}


Of course, it violates the Single Responsibility Principle by lumping everything into main().

### Ruby analogy

Since you're familiar with Ruby, let's use it to explore how to comply with the Single Responsibility Principle. This is roughly equivalent to the simple C code above.

def sum_comma_separated_numbers(filename)
sum, count = 0.0, 0
open(filename) do |file|
file.each do |line|
line.split(',').each do |num|
sum += num.to_f
count += 1
end
end
end
return sum / count
end

puts(sum_comma_separated_numbers('comma_separated.txt'))


To make the code above comply with the Single Responsibility Principle, I'd make an enumerator that yields floats.

class NumberTokenizer
include Enumerable

def initialize(io, sep=',')
@io = io
@sep = sep
end

def each
@io.each do |line|
line.split(@sep).each { |num| yield num.to_f }
end
end
end

def average(enum)
sum, count = 0.0, 0
enum.each do |num|
sum += num
count += 1
end
return sum / count
end

open('comma_separated.txt') do |f|
puts(average(NumberTokenizer.new(f)))
end


### Translating to C

Drawing inspiration from the Ruby example, we want to make a tokenizer that produces doubles, and an average() function that takes a tokenizer.

#include <stdio.h>

typedef struct {
FILE *f;
} Tokenizer;

Tokenizer *tokenizer_init(Tokenizer *t, FILE *f) {
t->f = f;
return t;
}

/* Returns 1 if there is a next comma-separated or
space-separated number, 0 if there is not. */
int tokenizer_next(Tokenizer *t, double *d) {
return 1 == fscanf(t->f, "%lf,", d);
}

double average(Tokenizer *t) {
double sum = 0.0;
int count = 0;
double d;
while (tokenizer_next(t, &d)) {
sum += d;
count++;
}
return sum / count;
}

int main(void) {
const char *filename = "comma_separated.txt";
FILE *f = fopen(filename, "r");
if (!f) {
perror(filename);
return 1;
}

Tokenizer t;
printf("average = %lf\n", average(tokenizer_init(&t, f)));
fclose(f);
return 0;
}


I would consider that a SRP-compliant solution that does not have any memory management headaches. (Of course, there is sophisticated buffer management going on behind the scenes within fscanf(), but it's nicely hidden from you.)

The tokenizer concept is actually just window dressing around fscanf(). You could simplify things with typedef FILE tokenizer, but I prefer to keep the abstraction there.

### Miscellaneous

While Ruby uses two spaces per level of indentation, C code generally has 4 or 8 spaces per level.

# Things you can improve

### Variables/Initialization

• Floating-point math is challenging in surprising places. It's easy to write down a reasonable algorithm that introduces 0.01% error on every step, which over 1,000 iteration turns the results into complete slop. You can easily find volumes filled with advice about how to avoid such surprises. Much of it is valid today, but much of it is easy to handle quickly: use double instead of float, and for intermediate values in calculations, it doesn't hurt to use long double.

The problem is how many bits each type uses to store significant digits. float uses 32 bits, and double uses 64 bits. 64 bits is enough to reliably store 15 significant digit, which is plenty in order to solve all of our floating-point problems for now. All we had to do was throw twice as much space at the problem, and suddenly all sorts of considerations are basically irrelevant. Even if there is a perceptible speed difference between a program written with all doubles and one written with all floats, it's worth extra microseconds to be able to ignore so many caveats.

• Don't use global variables.

int element_counter;
float* all_elements;


The problem with global variables is that since every function has access to these, it becomes increasingly hard to figure out which functions actually read and write these variables.

To understand how the application works, you pretty much have to take into account every function which modifies the global state. That can be done, but as the application grows it will get harder to the point of being virtually impossible (or at least a complete waste of time).

If you don't rely on global variables, you can pass state around between different functions as needed. That way you stand a much better chance of understanding what each function does, as you don't need to take the global state into account.

So instead of using global variables, initialize the variables in main(), and pass them as arguments to functions if necessary.

### Standards

• strtok is limited to tokenizing only one string (with one set of delimiters) at a time, and it can't be used while threading. Therefore, strtok is considered deprecated.

Instead, use strtok_r or strtok_s which are threading-friendly versions of strtok. The POSIX standard provided strtok_r, and the C11 standard provides strtok_s. The use of either is a little awkward, because the first call is different from the subsequent calls.

1. The first time you call the function, send in the string to be parsed as the first argument.

2. On subsequent calls, send in NULL as the first argument.

3. The last argument is the scratch string. You don't have to initialize it on first use; on subsequent uses it will hold the string as it is parsed so far.

To demonstrate its use, I've written a simple line counter (of only non-blank lines) using the POSIX standard one. I'll leave the choice of what version to use and implementation into your program up to you.

#include <string.h> // strtok_r

int countLines(char* instring)
{
int counter = 0;
char *scratch, *txt;
char *delimiter = "\n";
for (; (txt = strtok_r((!counter ? instring : NULL), delimiter, &scratch)); counter++);
return counter;
}


Also, in my use of strtok(), what should I be doing with the balance of the string after the delimiter? Is that hanging out in memory somewhere?

It's not hanging out in memory, it's in the scratch string you provided.

• You don't have to return 0 at the end of main(), just like you wouldn't bother putting return; at the end of a void-returning function. The C standard knows how frequently this is used, and lets you not bother.

C99 & C11 §5.1.2.2(3)

...reaching the } that terminates the main() function returns a value of 0.

### Syntax/Styling

• Put the variable declarations to separate lines.

float sum = 0.0, average;


From Code Complete, 2d Edition, p. 759:

With statements on their own lines, the code reads from top to bottom, instead of top to bottom and left to right. When you’re looking for a specific line of code, your eye should be able to follow the left margin of the code. It shouldn’t have to dip into each and every line just because a single line might contain two statements.

• Use strtod() instead of strtof(). (This goes with my point about double vs. float).

• Declare your parameters as void if you don't take in any arguments in your methods.

int main(void)

• You can simplify your NULL checks.

while(element)  // same as while (element != NULL)

• Initialize i inside of your for loops.(C99)

for (int i = 0; i < get_number_of_read_elements(); i++)