1
\$\begingroup\$

I am writing a program that is capable of reading a matrix supplied from a text file. One intended feature of this program is that it should be capable of detecting various delimiters in the text file.

So, for example, these inputs should be interpreted equally:

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

1:2:3:4
5:6:7:8
9:10:11:12

My code for detecting the delimiting character is as follows:

#include <stdio.h>
#include <limits.h>

static int isNumeric(unsigned char c)
{
    switch(c) {
    case '.':
    case '0' ... '9':
        return 1;
    default:
        return 0;
    }
}

unsigned char findDelimeter(FILE * f)
{
    rewind(f);
    int c;
    unsigned char delimeter = '\0';
    long long int frequencies[UCHAR_MAX] = { 0 };
    long long int maxFrequency = 0;
    while ((c = fgetc(f)) != EOF) {
        frequencies[c]++;
    }
    for (c = 0; c < UCHAR_MAX; c++) {
        if (frequencies[c] > maxFrequency && !(isNumeric(c) || c == '\n')) {
            maxFrequency = frequencies[c];
            delimeter = c;
        }
    }
    return delimeter;
}

I have a few concerns about this code:

  • Are the large data types, like long long int necessary in the findDelimeter function? Would it be better to use the types defined in stdint.h, like uint64_t? My intention was to support very large files.
  • How can I deal with the situation where an arbitrary number of spaces follow a delimiting character?

Regarding the last point, I would also like my code to be able to deal with text files that may use a space or a non-space delimiter followed by an arbitrary number of spaces. For example, the following inputs should be interpreted equally:

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

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

Currently, it detects the delimiter as the most frequent, non-numeric character. So in the last example, it would detect the space character as the delimiter since it is the most common non-numeric character, and it would mistakenly include the comma as part of the number when interpreting it. One solution that I thought of was modifying the findDelimeter function to return a string (er, character array), but I anticipate that would not be able to deal with an arbitrary number of spaces.

Also, as usual, I very much appreciate any stylistic comments or potential bugs.

\$\endgroup\$
5
  • 1
    \$\begingroup\$ Shouldn't you just make any non-digit character act as a delimiter? In your second example with the commas plus spaces, you would have to skip both the commas and the spaces to parse the digits. I guess it would be interesting to see how you plan on using the "delimiter" to parse the numbers. \$\endgroup\$
    – JS1
    Mar 1 '16 at 20:54
  • \$\begingroup\$ suggest allow the delimiter to be any non numberic value except newline. to find what the current delimiter is, read the first line, then step through characters until a number char found. the step through the numeric characters until a non number character is found. That non-numeric char is the delimiter \$\endgroup\$ Mar 1 '16 at 23:03
  • \$\begingroup\$ this will not compile: case '0' ... '9': a `case can only have one value. so the code needs a series of case statements with one case statement for each value of interest. \$\endgroup\$ Mar 1 '16 at 23:05
  • \$\begingroup\$ to make the code so it does not care about what the delimiter is; implement a state machine with 2 states. state1-not in a numeric string. state2-in a numeric string. transition from state 1 to state2 when prior state was state1 and numeric character encountered. transition from state2 to state1 when prior state was state2 and non-numeric char found,. \$\endgroup\$ Mar 1 '16 at 23:11
  • \$\begingroup\$ @user3629249 See Case Ranges, a GCC extension. \$\endgroup\$
    – Spikatrix
    Mar 13 '16 at 13:00
1
\$\begingroup\$

The first way to deal with this issue is to examine what is the actual problem that you're attempting to solve. Understand the problem fully before you solve it or you will end up with a mess. There are two parts to why you might need this function...

1) You want to process the data.

To perform processing you are looking to extract blocks of numbers. Knowing the delimiter is not relevant to your processing of the data. You're looking for 1 or more numbers which are contiguous and anything that is not a number can be ignored until you hit another number block.

That's your matrix processing taken care of without having to ever address any of your concerns or having a findDelimiter at all.

2) You want to reformat the file

If you want to output a formatted file with the same delimiter then your overall idea to identify a delimiter by building a frequency table and excluding certain values is on the right track.

To handle the issue of spaces, you can just set the [space] count to a maximum of 1 and if no other delimiters are present then then [space] will be the most frequent character.

There are some potential issues since you're dealing with arbitrary input. You have no guarantee the delimiter will remain constant throughout the file; it wouldn't be unreasonable to see two matrix files concatenated for example.

Equally you can avoid dealing with the very large datatypes if you limit your scanning to the first few kilobytes of a file. This would also provide a usable result without the overhead of processing the whole file; though you could argue that it's premature optimisation I would consider it justified in this case as you are expecting very large files.

Other potential issues...

This code assumes ASCII so far as I can tell. There could be some "undefined behaviour" should someone give this code a file with Unicode content.

You spelled delimiter as delimeter - while this shouldn't be a big issue as most IDE's autocomplete, it's worth keeping an eye on as if another coder worked on your code and spelled it properly there could be confusion. Many IDE's offer spellcheck and you should make sure yours is set up.

isNumeric will be an existing function. Though you could eliminate the need for it entirely by setting the frequency counts for characters you want to ignore (like 0...9 and control characters) to a frequency of 0 before you process the frequency table. This would also help remove or manage some of the 'magic numbers' in your code and make the intent clearer.

It's good practice to delay defining variables until you need them. This can be useful should you come to refactor the code as compiler errors will point out when you attempt to modify a variable before it has been initialised.

I'd like to see c and f have meaningful names and for there to be a few more blank lines to illustrate the program flow blocks - the steps of the process become more obvious with the blank lines (as they are here... build a frequency table, remove the unwanted values, de-emphasise space, find the most common term and return it). When the higher levels are clearly visible it makes refactoring easier.

unsigned char findDelimiter(FILE * fileToScan)
{
    rewind(fileToScan);

    int character;
    long long int frequencies[UCHAR_MAX] = { 0 };
    while ((character = fgetc(fileToScan)) != EOF) {
        frequencies[character]++;
    }

    const char IGNORECHARS[] = "0123456789\r\n";
    // todo: set the IGNORECHARS frequencies to 0 here
    // todo: set [space] to a maximum of 1 here

    long long int maxFrequency = 0;
    unsigned char delimiter = '\0';
    for (character = 0; character < UCHAR_MAX; character++) {
        if (frequencies[character] > maxFrequency) {
            maxFrequency = frequencies[character];
            delimiter = character;
        }
    }

    return delimiter;
}

With the code formatted this way it's easier to see how you could split the frequency analysis from the file reading should you want to do that later.

delimiter should be initialised with a reasonable value like ',' or be checked to throw an exception if a delimiter has not been found.

\$\endgroup\$
0
\$\begingroup\$
  1. Broken code: Too small an array by 1.

    // long long int frequencies[UCHAR_MAX] = { 0 };
    long long int frequencies[UCHAR_MAX + 1] = { 0 };
      ...
      ... c = fgetc(f) ...
      ...
      frequencies[c]++;
    ...
    // Test all `unsigned char`
    // for (c = 0; c < UCHAR_MAX; c++) {
    for (c = 0; c <= UCHAR_MAX; c++) {
      if (frequencies[c]
    
  2. Non-standard C code - I assume it works for OP.

    // case '0' ... '9':
    case '0':
    case '1':
    case '2':
    // seven more lines
    
  3. Minor: Might as well use unsigned types like unsigned long long or even better uintmax_t. IMO: Yes, "large data types" are necessary given the algorithm of finding the most popular non-numeric character.

    // long long int frequencies[UCHAR_MAX] = { 0 };
    unsigned long long frequencies[UCHAR_MAX] = { 0 };
    
  4. I would add '-' and '+' to the list of "numeric" characters.

  5. Suggest to invalidate all white space as a delimiter candidate. This will handle "How can I deal with the situation where an arbitrary number of spaces follow a delimiting character?".

     if (frequencies[c] > maxFrequency && !(isNumeric(c) || c == '\n')) 
     // if (frequencies[c] > maxFrequency && !(isNumeric(c) || isspace(c))) { 
    
  6. To detect problem cases (zero length file), return EOF.

    int  findDelimeter(FILE * f)
      int delimiter = EOF;
      for (c = 0; c <= UCHAR_MAX; c++) {
        if (frequencies[c] > maxFrequency ...
      ...
      return delimiter;
    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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