Skip to main content
added 60 characters in body
Source Link
ljrk
  • 919
  • 5
  • 11
  • 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("fopen()"__func__);
         exit(EXIT_FAILURE);
     }
    

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

     fprintf(stderr, "fopen(%s__func__ "%s, "r")\"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.

  • 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("fopen()");
         exit(EXIT_FAILURE);
     }
    

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

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

    This is much more useful information, imho

  • 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.

  • 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.

Source Link
ljrk
  • 919
  • 5
  • 11

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("fopen()");
         exit(EXIT_FAILURE);
     }
    

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

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

    This is much more useful information, imho

  • 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 ;)