Processing trip route table in C

Question

In fact, this is a group assignment for our C programming lesson, but seeking help from others is not considered cheating.

The goal of the following code is to:

1. Sort the data in start.txt by destination, and then by month if two trip routes share one destination.
2. Count how many trip routes each month and destination have.
3. Output the sorted data and the statistics to result.txt.

Since this is my first project, I want its coding style to be reviewed most of all. Also, please let me know if adding register or restricted somewhere can be benefitial, for I want to learn more about the use of modifiers in C. Of course, other optimisations are appreciated too.

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

#define MAX 500

typedef struct info
{
    unsigned num;
    char dest[80];
    unsigned length;
    char cat[80];
    unsigned month;
    unsigned price;
} info;

size_t readIn(info *s);
void   overallSort(info *s, size_t itemNum);
size_t groupByDest(info *s, size_t firstItemIndex, size_t itemNum);
void   sortByMonth(info *s, size_t lowerBound, size_t upperBound);
void   countRouteMonth(info *s, size_t itemNum, unsigned *numEachMonth);
void   countRouteKind(info *s, size_t itemNum, unsigned *numEachDest);
void   output(info *s, size_t itemNum, unsigned *numEachMonth, unsigned *numEachDest);

int main(void)
{
    size_t itemNum;
    struct info s[MAX];
    unsigned numEachMonth[12];
    unsigned numEachDest[MAX] = {0};
    itemNum = readIn(s);
    overallSort(s, itemNum);
    countRouteMonth(s, itemNum, numEachMonth);
    countRouteKind(s, itemNum, numEachDest);
    output(s, itemNum, numEachMonth, numEachDest);
    setlocale(LC_ALL, "");
    fwide(stdout, 1);
    fputwc(L'\uFEFF', stdout);
    wprintf(L"\u4E34\u7801\u6D95\u96F6\uFF0C\u4E0D\u77E5\u6240\u8A00\u3002\n");
    wprintf(L"\u5E78\u751A\u81F3\u54C9\uFF0C\u6B4C\u4EE5\u548F\u5FD7\u3002\n");
    return EXIT_SUCCESS;
}

size_t readIn(struct info *s)
{
    unsigned itemCount = 0;
    FILE *fp;
    if((fp = fopen("start.txt", "r")) == NULL)
    {
        perror("Opening start.txt");
        exit(EXIT_FAILURE);
    }
    char buffer[80];
    while(fgets(buffer, 80, fp))
    {
        sscanf(buffer, " %u %s %u %s %u %u", &s[itemCount].num, s[itemCount].dest, &s[itemCount].length, s[itemCount].cat, &s[itemCount].month, &s[itemCount].price);
        itemCount++;
    }
    fclose(fp);
    return itemCount;
}

void overallSort(info *s, size_t itemNum)
{
    size_t destIndex = 0;
    while(destIndex < itemNum)
    {
        destIndex += groupByDest(s, destIndex, itemNum);
    }
}

size_t groupByDest(info *s, size_t firstItemIndex, size_t itemNum)
{
    info temp;
    size_t i, j = firstItemIndex + 1, groupSize = 1;
    for(i = firstItemIndex + 1; i < itemNum; i++)
    {
        if(strcmp(s[i].dest, s[firstItemIndex].dest) == 0)
        {
            if(i != j)
            {
                temp = s[j];
                s[j] = s[i];
                s[i] = temp;
            }
            j++;
            groupSize++;
        }
    }
    sortByMonth(s, firstItemIndex, firstItemIndex + groupSize - 1);
    return groupSize;
}

void sortByMonth(info *s, size_t lowerBound, size_t upperBound)
{
    info temp;
    size_t i, j;
    for(i = lowerBound; i < upperBound; i++)
    {
        for(j = lowerBound; j < upperBound; j++)
        {
            if(s[j].month > s[j+1].month)
            {
                temp = s[j];
                s[j] = s[j + 1];
                s[j + 1] = temp;
            }
        }
    }
}

void countRouteMonth(info *s, size_t itemNum, unsigned *numEachMonth)
{
    size_t i, j;
    for(i = 0; i < itemNum; i++)
    {
        for(j = 0; j < 12; j++)
            if(s[i].month == j + 1)
            {
                numEachMonth[j]++;
            }
    }
}

void countRouteKind(info *s, size_t itemNum, unsigned *numEachDest)
{
    size_t i, num = 1, DestIndex = 0;
    for(i = 0; i < itemNum; i++)
    {
        if(strcmp(s[i].dest, s[i + 1].dest) == 0)
        {
            num++;
        }
        else
        {
            numEachDest[DestIndex] = num;
            DestIndex++;
            num = 1;
        }
    }
}

void output(struct info *s, size_t itemNum, unsigned *numEachMonth, unsigned *numEachDest)
{
    size_t i, destIndex = 0;
    FILE *fp;
    if((fp = fopen("result.txt", "w")) == NULL)
    {
        perror("Opening result.txt");
        exit(EXIT_FAILURE);
    }

    fprintf(fp, "Number\tDest\tDays\tCat\t\tMonth\tPrice\n");
    for(i = 0; i < itemNum; i++)
    {
        fprintf(fp, "%u\t%s\t%u\t%s\t%u\t%u\n", s[i].num, s[i].dest, s[i].length, s[i].cat, s[i].month, s[i].price);
    }

    fputc('\n', fp);
    fprintf(fp, "STATISTICS:\n");
    fputc('\n', fp);
    for(i = 0; i < 12; i++)
    {
        fprintf(fp, "The number of the routes in month %zu is %u.\n", i + 1, numEachMonth[i]);
    }
    fputc('\n', fp);
    for(i = 0; numEachDest[i]; i++)
    {
        fprintf(fp, "The number of the routes to %s is %u.\n", s[destIndex].dest, numEachDest[i]);
        destIndex += numEachDest[i];
    }
    fclose(fp);
}

start.txt contains:

1 Guilin  7   Domestic    2   9857
2 Suzhou  7   Domestic    1   47893
3 Wuxi    3   Domestic    3   41525
4 Tibet   4   Domestic    4   13123
5 Guilin  5   Domestic    7   5532
6 Wuxi    6   Domestic    8   232
7 Suzhou  7   Domestic    9   132132
8 Wuxi    8   Domestic    4   343432
9 Suzhou  9   Domestic    12  24234
10    Jinan   10  Domestic    5   42334
11    Wuxi    7   Domestic    6   15156
12    Tibet   3   Domestic    10  25425
13    Tibet   4   Domestic    11  35345
14    Tibet   5   Domestic    12  34453
15    Suzhou  6   Domestic    3   34345
16    Jinan   7   Domestic    5   8785
17    Suzhou  8   Domestic    6   334533
18    Jinan   9   Domestic    7   4653
19    Wuxi    10  Domestic    9   34234
20    Wuxi    7   Domestic    10  2432423
21    Jinan   3   Domestic    1   43432
22    Tibet   4   Domestic    11  87678
23    Suzhou  5   Domestic    5   78878
24    Suzhou  6   Domestic    7   7826
25    Jinan   7   Domestic    12  2132
26    Tibet   8   Domestic    3   464656
27    Wuxi    9   Domestic    8   9857
28    Tibet   10  Domestic    9   47893
29    Tibet   7   Domestic    2   41525
30    Jinan   3   Domestic    6   13123
31    Suzhou  4   Domestic    7   5532
32    Guilin  5   Domestic    3   232
33    Guilin  6   Domestic    4   132132
34    Guilin  7   Domestic    5   343432
35    Wuxi    8   Domestic    9   24234
36    Guilin  9   Domestic    10  42334
37    Suzhou  10  Domestic    11  15156
38    Wuxi    7   Domestic    2   25425
39    Suzhou  3   Domestic    3   35345
40    Guilin  4   Domestic    6   34453
41    Munich  5   International       7   34345
42    Dubai   6   International       5   8785
43    London  7   International       6   334533
44    Paris   8   International       9   4653
45    Vienna  9   International       8   34234
46    Moscow  10  International       11  2432423
47    Berlin  7   International       12  43432
48    Tokyo   3   International       10  87678
49    Sydney  4   International       9   78878
50    Madrid  5   International       8   7826
51    Munich  6   International       7   2132
52    Dubai   7   International       6   464656
53    London  8   International       5   9857
54    Paris   9   International       4   47893
55    Vienna  10  International       7   41525
56    Moscow  7   International       5   13123
57    Berlin  3   International       3   5532
58    Tokyo   4   International       2   232
59    Sydney  5   International       1   132132
60    Madrid  6   International       4   343432
61    Munich  7   International       5   24234
62    Dubai   8   International       6   42334
63    London  9   International       7   15156
64    Paris   10  International       9   25425
65    Vienna  7   International       1   35345
66    Moscow  3   International       2   34453
67    Berlin  4   International       10  34345
68    Tokyo   5   International       11  8785
69    Sydney  6   International       12  334533
70    Madrid  7   International       11  4653
71    Munich  8   International       9   34234
72    Dubai   9   International       5   2432423
73    London  10  International       6   43432
74    Paris   7   International       7   87678
75    Vienna  3   International       2   78878
76    Moscow  4   International       3   7826
77    Berlin  5   International       4   2132
78    Tokyo   6   International       5   464656
79    Sydney  7   International       8   9857
80    Madrid  8   International       9   47893
81    Munich  9   International       10  41525
82    Dubai   10  International       3   13123
83    London  7   International       4   5532
84    Paris   3   International       5   232
85    Vienna  4   International       8   132132
86    Moscow  5   International       4   343432
87    Berlin  6   International       8   24234
88    Tokyo   7   International       1   42334
89    Sydney  8   International       7   15156
90    Madrid  9   International       7   25425
91    Munich  10  International       7   35345
92    Dubai   7   International       7   34453
93    London  3   International       4   34345
94    Paris   4   International       3   8785
95    Vienna  5   International       2   334533
96    Moscow  6   International       6   4653
97    Berlin  7   International       7   34234
98    Tokyo   8   International       9   2432423
99    Sydney  9   International       8   43432
100   Madrid  10  International       10  87678

Answer 1

Additionally to the variable-naming that was already mentioned:

• Put "magic constants" into defines:
  • #define BUFLEN 80, #define INPUT_FILE "start.txt" etc.
• readIn() etc. will perhaps in future be re-used with different filenames -- so prefer giving them as a parameter rather than relying on globals.

• Preference: Use function name in parameter to perror(), eg.:

  if((fp = fopen(INPUT_FILE, "r")) == NULL) {
      perror(__func__);
      exit(EXIT_FAILURE);
  }
  

  if you want to not only display the function called you can use.

  fprintf(stderr, __func__ "%s, \"r\"": %s\n", filename, strerror(errno));
  

  This is much more useful information, imho (Thanks to @chux for the hint with the __func__-macro)

• I find this hard to read:

  size_t i, j = firstItemIndex + 1, groupSize = 1;
  

  Why not seperate the declarations and put i into the following for statement (not only here)?

  size_t j = firstItemIndex +1, groupSize = 1;
  for (size_t i = firstItemIndex +1, ...
  

  • Also I find the logic here hard to folow -- why are you initializing i to the value of firstItemIndex + 1 + 1? This might make sense but not without trying to go through your code line-by-line.

• strcmp() compares two strings -- and if they don't differ it will go on, until it reads the terminating '\0'-character. Sadly, if both strings are longer than their buffer its results are unpredictable. Use strncmp() instead and give it a maximum length to compare!

• Be consistent: sometimes you use struct info, sometimes just info. As the latter is typedef-ed to the former you could always omit the struct. This is personal preference but use either (I prefer not omitting the struct if I'm not dealing with many as this obfuscates code less, imho again).

• Separate different code sections like processing, output, etc. with empty lines. This makes it more easy to understand that these are distinct. Example: Add a newline after output(...) in main().

• You are iterating using a size_t type but writing the result to an unsigned:

  $ gcc -std=c99 -Wall -Wextra -Wpedantic -Wconversion -Wno-sign-compare -Wshadow main.c -o main
  main.c: In function ‘countRouteKind’:
  main.c:133:29: warning: conversion to ‘unsigned int’ from ‘size_t {aka long unsigned int}’ may alter its value [-Wconversion]
      numEachDest[DestIndex] = num;
                               ^~~
  

• [PERSONAL PREFERENCE] Use unsigned int instead of just unsigned.

• [PERSONAL PREFERENCE] Variable naming: You use "camelCase". In C usually "snake_case" is preferred. It's nothing bad, but I like to stick with the style a language has as it makes things more consistent.

• [PERSONAL PREFERENCE] Most C programmers I know prefer code to have a limited line width (like 80 chars). While modern screens of course easily fit more than this limit, it makes it nicer to have multiple editors of code open. If you think 80 chars is not enough, set your limit to 100, 120 or whatever your institution uses.

Otherwise I think you did good -- it's definitely not what I'd consider ugly code or similar, don't be intimidated by the number of changes I suggest, these are just minor things ;)

Answer 2

Naming

Structure/variable naming is one of the most effective ways you have of making your code self documenting. It's always worth taking some time to consider your names, this becomes more important the wider the usage of the names. You have names like s, info, num for your most important structure, its members and references to it. These are far from descriptive and are worth reconsidering.

You also have method names like readin, output, what are these methods reading/writing, to where?

Overflow

You readin method loops through a file, reading into a constantly sized structure buffer. If the file contains more lines than the number of buffer elements it will overflow. You should be checking that itemCount < MAX.

Error checking

Your readin method is using sscanf to parse the contents of the file. You're not checking it's return value, what are you expecting it to do if you get passed a line that's not the correct format?

Bracing

This is subjective, but I don't really like this:

for(j = 0; j < 12; j++)
    if(s[i].month == j + 1)
    {

It's error prone if you decide you need to extra behaviour to the for loop and forget to add the braces when you do. I'd prefer to see braces wrapping the for loop...