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 offirstItemIndex + 1 + 1
? This might make sense but not without trying to go through your code line-by-line.
- Also I find the logic here hard to folow -- why are you initializing
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. Usestrncmp()
instead and give it a maximum length to compare!Be consistent: sometimes you use
struct info
, sometimes justinfo
. As the latter istypedef
-ed to the former you could always omit thestruct
. 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(...)
inmain()
.You are iterating using a
size_t
type but writing the result to anunsigned
:$ 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 justunsigned
.[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 offirstItemIndex + 1 + 1
? This might make sense but not without trying to go through your code line-by-line.
- Also I find the logic here hard to folow -- why are you initializing
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. Usestrncmp()
instead and give it a maximum length to compare!Be consistent: sometimes you use
struct info
, sometimes justinfo
. As the latter istypedef
-ed to the former you could always omit thestruct
. 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(...)
inmain()
.You are iterating using a
size_t
type but writing the result to anunsigned
:$ 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 justunsigned
.[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 offirstItemIndex + 1 + 1
? This might make sense but not without trying to go through your code line-by-line.
- Also I find the logic here hard to folow -- why are you initializing
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. Usestrncmp()
instead and give it a maximum length to compare!Be consistent: sometimes you use
struct info
, sometimes justinfo
. As the latter istypedef
-ed to the former you could always omit thestruct
. 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(...)
inmain()
.You are iterating using a
size_t
type but writing the result to anunsigned
:$ 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 justunsigned
.[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.
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 offirstItemIndex + 1 + 1
? This might make sense but not without trying to go through your code line-by-line.
- Also I find the logic here hard to folow -- why are you initializing
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. Usestrncmp()
instead and give it a maximum length to compare!Be consistent: sometimes you use
struct info
, sometimes justinfo
. As the latter istypedef
-ed to the former you could always omit thestruct
. 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(...)
inmain()
.You are iterating using a
size_t
type but writing the result to anunsigned
:$ 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 justunsigned
.[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 ;)