Analyzer for compression algorithms (or tools)

Question

This file contains the main functions of my tool, rac. In particular, I care a lot about the Shannon function.

Would you please give some hints to make the file cleaner? I have a perversion for concise and good indented code.

#include <math.h>
#include <stdio.h>
#include "headers/utils.h"

double ccount(char * foo)
{
  FILE * file = fopen(foo, "r");
  int a = 0, b;
  while ((b = getc(file)) != EOF)
    a++;

  fclose(file);
  return a;
}

int cmp(double foo, double bar)
{
  if (foo >= bar)
    return 1;
  else
    return 0;
}

double ratio(double foo, double bar)
{
  return (100*(foo - bar)/ foo);
}

void stat(char * file, double vector[256])
  {
    int c;
    FILE * ftopen = fopen(file, "r");
    while ((c = getc(ftopen)) != EOF)
      {
        if (c >= 0 && c <= 256) 
            vector[c]++;  
      }

      fclose(ftopen);
  }

void freq(char * foo, double vector[256])
{
  int i;
  for (i = 0; i < 256; i++)
  {
    if (vector[i] == 0)
      continue;
    else
      {
        vector[i] = (vector[i] / ccount(foo));
      }
  }
}

double shannon(double vector[256])
{
  int i;
  double shan = 0.0;
  for (i = 0; i < 256; i++)
  {
    if (vector[i] == 0.0)
      continue;
    else
      {
        shan = shan + (log2(pow(vector[i], vector[i]))); 
      }
  }
  return (shan * -1.0); //entropy of the file
}

Answer 1

MAGIC NUMBERS
The number 256 is used in multiple places as an array size. Rather than using the raw number 256, it would be better to use a symbolic constant. Raw numbers in code are sometimes referred to as magic numbers, because it isn't clear what they mean. There are at least two reasons for this,

1. It identifies what the number means.
2. If at some point in the future the number has to be changed for all uses, the code only needs to be changed in one place.

An Example in C:

#define ARRAY_SIZE 256

void stat(char * file, double vector[ARRAY_SIZE])
{
    int c;
    FILE * ftopen = fopen(file, "r");
    while ((c = getc(ftopen)) != EOF)
      {
        if (c >= 0 && c <= ARRAY_SIZE)
            vector[c]++;
      }

      fclose(ftopen);
}

An Example in C++:

constexpr int ARRAY_SIZE = 256;

Use Meaningful Variable and Function Names
The code contains foo and bar in a number of places. These are not meaningful names. The names should indicate what the variables are, so that if you or someone else needs to change the code 2 years in the future it is an easy task rather than having to go through all the code to see what needs to be changed.

Another exmaple of this is the function ccount(), which could be renamed file_size().

A third example is the array vector. Other than array if doubles it is not clear what this array is used for.

Inconsistent Coding Style
In a professional setting where multiple people may have to maintain the code it is best to be as consistent as possible.

The else clauses almost always have { block}` but the if statements do not. This is true whether or not the block contains multiple statement or not. You should choose one way or the other.

In the for loops the { is immediately under the for, but in the else clauses it is indented. Choose one or the other. The most common usage is how you do the for loops.

This is partially a matter of choice, I use { block } on all control statements:

if ()
{
}
else
{
}

for ()
{
}

while ()
{
}

do
{
} while ();

I do this even for single statements because if the code needs to be updated by adding a statement to a block it is easier for anyone to see where the line of code should be inserted. This was also required as part of a coding standard at a company where I worked.

Empty Clauses in IF/ELSE
In the function shannon() the code contains:

    if (vector[i] == 0.0)
      continue;
    else
      {
        shan = shan + (log2(pow(vector[i], vector[i])));
      }

This would be clearer if it was written

    if (vector[i] != 0.0)
      {
        shan = shan + (log2(pow(vector[i], vector[i])));
      }

An else clause can be added at a later time if it is needed. The if portion of the current implementation is meaningless and would be optimized out by the compiler.

Answer 2

In addition to what @pacmaninbw already said:

The return value of fopen() is not checked. It will be NULL if the file does not exist or the caller has no read permission. You have to decide how to handle that. Another option would be to open the file in main() and pass the file handle to your functions.

A simpler method to get the size of a file is using stat() (or fstat() if you already have a handle to the file). This is also much faster, because it only queries the filesystem, without reading the entire file.

The cmp() function can be simplified to

int cmp(double foo, double bar)
{
  return foo >= bar;
}

because in C, the result of a comparison operator is either 0 or 1. In fact that is so simple that you probably don't need a dedicated function for that purpose.

The return value of getc() is either EOF, or an unsigned char converted to an int. In other words, if a char has 8 bit (which is the case on all Posix-compatible platforms) then the return value can only be EOF or an integer in the range 0 ... 255, so there is no need for a check

if (c >= 0 && c <= 256)

(and you probably meant c < 256 here).

If you cannot assume that a char has 8 bits then use 1 << CHAR_BIT as the array size, not 256.

In the freq() function, at

    vector[i] = (vector[i] / ccount(foo));

the file size it computed for each loop iteration. Better compute it once and store it in a local variable.

In the shannon() function

    shan = shan + (log2(pow(vector[i], vector[i]))); 

should be (as I understand it from https://en.wikipedia.org/wiki/Entropy_(information_theory)#Definition)

    shan = shan + log2(vector[i]) * vector[i];