# Summarizing calories for a food diary

I'm new to C and I've been working on some code for a project for school. This was the finished result and now that I'm done it, I came back and tried making it better. So I was wondering, how would I make this printAvgCalPerGroup simplified?

#include "diary.h"

void printTotalCal(Food * list)
{
int total = 0;
Food * temp = list;

while (!isEmpty(temp))
{
total += (int)temp->calories;
temp = temp->next; // change to the next list item
}

printf("%d\n", total);
return;
}

void printAvgCalPerGroup(Food * list)
{
double vegCount = 0.0;
double meatCount = 0.0;
double dairyCount = 0.0;
double grainsCount = 0.0;
double fatCount = 0.0;

double vAmnt = 0.0;
double mAmnt = 0.0;
double dAmnt = 0.0;
double gAmnt = 0.0;
double fAmnt = 0.0;

Food * temp = list;

while (temp!= NULL)
{
if (temp->foodGroup == 'v')
{
vAmnt+=1.0;
vegCount+=temp->calories;
}
else if (temp->foodGroup == 'm')
{
mAmnt+=1;
meatCount+=temp->calories;
}
else if (temp->foodGroup == 'd')
{
dAmnt+=1;
dairyCount +=temp->calories;
}
else if (temp->foodGroup == 'g')
{
gAmnt+=1;
grainsCount+=temp->calories;
}
else if (temp->foodGroup == 'f')
{
fAmnt+=1;
fatCount+=temp->calories;
}
temp = temp->next;
}
printf("%.2f\n", calcAvg(vegCount,vAmnt));
printf("%.2f\n", calcAvg(meatCount,mAmnt));
printf("%.2f\n", calcAvg(dairyCount,dAmnt));
printf("%.2f\n", calcAvg(grainsCount,gAmnt));
printf("%.2f\n", calcAvg(fatCount,fAmnt));

}

double calcAvg(double x, double y)
{
double ans = x/y;
if (ans != ans) return 0.0;
return ans;
}

void run(char * argv)
{
Food * f = getDiary(argv);
printTotalCal(f);
printAvgCalPerGroup(f);
printList(f);
destroyList(f);
}


## Reducing repetition

The big if statement in printAvgCalPerGroup is a bit repetitive (as you pointed out). Imagine if you increased the number of food groups. You would have to copy/paste 4 places for each food group you added.

First, I would suggest using arrays for the counts and amounts of each foodgroup. Then you only have to convert the letter such as 'v' into an array index.

Second, if the food groups could possibly change, I would suggest using an enum to keep track of them. This is definitely optional, and you could use hardcoded numbers instead (e.g. 'v' -> 0, 'm' -> 1, etc).

Third, a small change to make would be to track the amounts using ints instead of doubles, since the amounts always increase by 1 only.

Here is what your code would look like using arrays and enums:

typedef enum FoodGroup {
VEG,
MEAT,
DAIRY,
GRAINS,
FAT,
FOODGROUP_MAX
} FoodGroup;

void printAvgCalPerGroup(Food * list)
{
double    count[FOODGROUP_MAX] = {0};
int       amnt [FOODGROUP_MAX] = {0};
FoodGroup group;

Food * temp = list;

while (temp!= NULL) {
switch (temp->foodGroup) {
case 'v': group = VEG;           break;
case 'm': group = MEAT;          break;
case 'd': group = DAIRY;         break;
case 'g': group = GRAINS;        break;
case 'f': group = FAT;           break;
default : group = FOODGROUP_MAX; break;
}
if (group < FOODGROUP_MAX) {
amnt [group]++;
count[group] += temp->calories;
}
temp = temp->next;
}

for(group=0; group<FOODGROUP_MAX; group++)
printf("%.2f\n", calcAvg(count[group], amnt[group]));
}


If you don't need the enum, just replace VEG with 0, MEAT with 1, etc.

• That's exactly what I wanted thanks, I didn't know about the enum being 0,1,2 etc so that was informative Apr 3, 2015 at 2:00
• You could even avoid the switch by creating an array of FoodGroups, initializing it one time, and do the lookup by using temp->foodGroup as the array reference. Apr 3, 2015 at 2:21

Having many similarly used variables suggests using an array:

    double calories[FOOD_CATEGORIES];
double amount[FOOD_CATEGORIES];


and some means of translating foodGroup to an index. In fact, I recommend to translate them right at the reading phase. Then the code collapses to

    while (temp != 0) {
calories[temp->food_index] += temp->calories;
amount[temp->food_index] += 1;
temp = temp->next;
}

for (int i = 0; i < FOOD_CATEGORIES]; i++)
printf("%.2f\n", calcAvg(calories[i], amount[i]));
}


That said, printAvgCalPerGroup does too much: it computes and prints. These independent responsibilities shall be given to independent functions.

• Why would you use multiple arrays when you have the option of making an array of structs? Apr 3, 2015 at 2:19
• @Snowbody For clarity (a baby step at a time).
– vnp
Apr 3, 2015 at 2:25

It looks like Food (which is not defined, you forgot to paste in diary.h) has a pointer to another Food object in the next member. This seems to be an attempt to implement a linked list (or a stack).

However, the bigger question is why do you use this method of storing multiple Food items? You aren't doing anything that benefits from having a linked list like adding things in the middle. What's the reason for not using a standard array or dynamically allocated memory?

• It was an assignment and that's the way we were told to do it for the assignment Apr 3, 2015 at 1:58
• And the linked list files are in a separate c file, I just wanted to know how to make this code shorter Apr 3, 2015 at 1:59

How about using alternative switch when you have a lot of if-else conditions?

Use data type int for the amount variables instead of double.

It is very simplified already.