Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Task:

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;

void mark_element_as_read() {
  element_counter += 1;
}

int get_number_of_read_elements() {
  return element_counter;
}

float* expand_array_stack_for(float* elements) {
  return realloc(elements, get_number_of_read_elements() * sizeof(elements));
}

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

float* get_collection() {
  return all_elements;
}

float* read_from_file(char* filename) {
  FILE *file_opened;

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

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

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

  free(element);
  fclose(file_opened);

  return get_collection();
}

int main() {
  float sum = 0.0, average;
  float *elements = read_from_file("comma_separated.txt");
  int i = 0;

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

  average = sum / get_number_of_read_elements();

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

  return 0;
}

I never coded in C before. This is my very first time. I used to code in Ruby, Java and PHP, so maybe this code would be a little bit "high leveled". Any tips will be very welcome!

share|improve this question

2 Answers 2

up vote 6 down vote accepted

Your C solution

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.

share|improve this answer

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;
    }
    

    If you implement this, you have your question answered.

    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++)
    
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.