7
\$\begingroup\$

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);
}
\$\endgroup\$
8
\$\begingroup\$

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.

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

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.

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

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?

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

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.