Summarizing calories for a food diary

Question

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[0] == 'v')
          {
               vAmnt+=1.0;
               vegCount+=temp->calories;
          }
          else if (temp->foodGroup[0] == 'm')
          {
               mAmnt+=1;
               meatCount+=temp->calories;
          }
          else if (temp->foodGroup[0] == 'd')
          {
               dAmnt+=1;
               dairyCount +=temp->calories;
          }
          else if (temp->foodGroup[0] == 'g')
          {
               gAmnt+=1;
               grainsCount+=temp->calories;
          }
          else if (temp->foodGroup[0] == '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);
}

Answer 1

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[0]) {
            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.

Answer 2

Having many similarly used variables suggests using an array:

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

and some means of translating foodGroup[0] 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.

Answer 3

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?

Answer 4

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.