Improving complex code (featuring: functions)

Andrew Bennett <andrew.bennett@unsw.edu.au>, COMP1511 19T1

Here's some code that I've written, which very vaguely approximates what you may or may not do in the assignment.

Consider that, rather than playing a card game, you're trying to manage a world full of delicious fruit and vegetables.

You want to write a function that will choose the best possible fruit for you to eat, based on whatever criteria, and that function should print out the best fruit at the end.

I've written three different versions of the code:

All of the code in one massive function
Some of the code moved to another function, but which could be improved
Some of the code moved to another function, in a more sensible way.

All of the code in one massive function:

(this was very difficult for me to write)

Note: this code contains many bugs.

#include <stdio.h>

#define SUMMER 0
#define AUTUMN 1
#define WINTER 2
#define SPRING 3

#define MAX_FOOD 1000

int is_in_season(int food);
void print_chosen_fruit(int fruit);

void choose_fruit_to_eat(void) {
    int num_fruits;
    int num_veges;
    int current_season;
    int fruits[1000];
    int veges[1000];

    scanf("%d %d %d", &num_fruits, &num_veges, &current_season);

    // Scan in the fruit to the fruits array.
    int i = 0;
    while (i < num_fruits) {
        // [scan the fruit into the array]
        i++;
    }

    // Scan in the vegetables to the veges array.
    i = 0;
    while (i < num_veges) {
        // [scan the veges into the array]
        i++;
    }

    // spring is the best season
    if (current_season == 3) {
        int fruits_in_season[MAX_FOOD];
        int veges_in_season[MAX_FOOD];
        int num_fruits_in_season;
        int num_veges_in_season;

        // Work out which fruit are in season
        i = 0;
        while (i < num_fruits) {
            if (is_in_season(fruits[i]) == 1) {
                // [copy to the fruits_in_season array somehow]
            }
            i++;
        }

        // Work out which veges are in season
        i = 0;
        while (i < num_veges) {
            if (is_in_season(veges[i]) == 1) {
                // [copy to the veges_in_season array somehow]
            }
            i++;
        }

        i = 0;
        while (i < num_fruits) {
            // 2 means it's in season
            if (fruits_in_season[i] == 2) {
                num_fruits_in_season++;
            }
            i++;
        }

        i = 0;
        while (i < num_veges) {
            if (veges_in_season[i] == 2) {
                num_veges_in_season++;
            }
            i++;
        }

        // Order the foods from best to worst
        // [pretend there's two more loops here to do this]

        int found_best_fruit = 0;

        if (num_fruits_in_season > 0) {
            i = 0;
            while (i < num_fruits_in_season) {
                // Compare each of the fruits in season to the best vegetable
                if (fruits_in_season[i] * veges_in_season[0] > 10000) {
                    print_chosen_fruit(fruits_in_season[i]);
                    found_best_fruit = 1;
                }
                i++;
            }

            // If we didn't find any great combinations, just find a
            // decent combination
            if (found_best_fruit == 0) {
                // If we only have one possible fruit, just pick that
                if (num_fruits_in_season == 1) {
                    print_chosen_fruit(fruits_in_season[0]);
                    found_best_fruit = 1;
                } else {
                    i = 0;
                    while (i < num_fruits_in_season && found_best_fruit == 0) {
                        if (fruits_in_season[i] * veges_in_season[0] > 10) {
                            print_chosen_fruit(fruits_in_season[i]);
                            found_best_fruit = 1;
                        }
                        i++;
                    }
                }
            }
        }
        // If we haven't found a good fruit yet, just check all of the
        // fruit (ignoring what's in season right now)
        if (found_best_fruit == 0) {
            while (i < num_fruits) {
                if (fruits[i] * veges_in_season[0] > 25000) {
                    print_chosen_fruit(fruits_in_season[0]);
                    found_best_fruit = 1;
                }
                i++;
            }
        }
        // If we stil haven't found a fruit, just print the first fruit.
        if (found_best_fruit == 0) {
            print_chosen_fruit(fruits[0]);
            found_best_fruit = 1;
        }
    } else if (current_season == 2) {
        // season 2 = winter.
        // winter has terrible fruits, so we don't print anything
        // (out of spite)
    } else {

        int fruits_in_season[MAX_FOOD];
        int veges_in_season[MAX_FOOD];
        int num_fruits_in_season;
        int num_veges_in_season;

        i = 0;
        while (i < num_fruits) {
            if (is_in_season(fruits[i]) == 1) {
                // [copy to the fruits_in_season array somehow]
            }
            i++;
        }

        i = 0;
        while (i < num_veges) {
            if (is_in_season(veges[i]) == 1) {
                // [copy to the veges_in_season array somehow]
            }
            i++;
        }

        i = 0;
        while (i < num_fruits) {
            // 2 means it's in season
            if (fruits_in_season[i] == 2) {
                num_fruits_in_season++;
            }
            i++;
        }

        i = 0;
        while (i < num_veges) {
            if (veges_in_season[i] == 2) {
                num_veges_in_season++;
            }
            i++;
        }

        int found_best_fruit = 0;


        while (i < num_fruits) {
            if (fruits[i] * veges_in_season[0] > 25000) {
                print_chosen_fruit(fruits[i]);
                found_best_fruit = 1;
            }
            i++;
        }
        if (found_best_fruit == 0) {
            print_chosen_fruit(fruits[0]);
            found_best_fruit = 1;
        }
    }
}

This code isn't terrible -- it has a few comments, the variable names are sensible, the indentation and whitespace are correct.

However, there's a lot more to style than just having comments, good variable names, and correct indentation + whitespace... and on the whole, this code is pretty shocking.

It wouldn't get 0% for style, but it also wouldn't be anywhere near full marks.

There are still a lot of improvements that we can make.

Code moved to another function, initial attempt

This is a first attempt at moving some of the code into functions.

It's a lot better than before, but there's still room for improvement.

#include <stdio.h>

#define SUMMER 0
#define AUTUMN 1
#define WINTER 2
#define SPRING 3

// Spring is the best season for fruits and veges
// (note: I made this up, I am not an expert on fruit or veges)
#define PREFERRED_SEASON SPRING

#define UNKNOWN -1
#define MAX_FOOD 1000

#define NOTHING -100

#define IN_SEASON 2

int is_in_season(int food);
void print_chosen_fruit(int fruit);

int choose_best_seasonal_fruit(
        int num_fruits,
        int fruits[],
        int num_veges,
        int vegetabes[],
        int num_fruits_in_season,
        int fruits_in_season[],
        int num_veges_in_season,
        int veges_in_season[],
        int current_season);


int choose_best_non_seasonal_fruit(
        int num_fruits,
        int fruits[],
        int num_veges,
        int vegetabes[],
        int num_fruits_in_season,
        int fruits_in_season[],
        int num_veges_in_season,
        int veges_in_season[],
        int current_season);

void choose_fruit_to_eat(void) {

    int num_fruits;
    int num_veges;
    int current_season;
    int fruits[MAX_FOOD];
    int veges[MAX_FOOD];
    int num_fruits_in_season = 0;
    int num_veges_in_season = 0;
    int fruits_in_season[MAX_FOOD];
    int veges_in_season[MAX_FOOD];


    scanf("%d %d %d", &num_fruits, &num_veges, &current_season);

    int i = 0;
    while (i < num_fruits) {
        // scan the fruit into the array
        i++;
    }

    i = 0;
    while (i < num_veges) {
        // scan the veges into the array
        i++;
    }

    int best_fruit = UNKNOWN;

    if (current_season == PREFERRED_SEASON) {
        best_fruit = choose_best_seasonal_fruit(
            num_fruits, fruits,
            num_veges, veges,
            num_fruits_in_season, fruits_in_season,
            num_veges_in_season, veges_in_season,
            current_season);
    } else if (current_season != WINTER) {
        // winter has terrible fruits
        best_fruit = NOTHING;
    } else {
        best_fruit = choose_best_non_seasonal_fruit(...);
    }

    print_chosen_fruit(best_fruit);
}

void choose_best_seasonal_fruit(
        int num_fruits,
        int fruits[],
        int num_veges,
        int veges[],
        int num_fruits_in_season,
        int fruits_in_season[],
        int num_veges_in_season,
        int veges_in_season[],
        int current_season) {

    find_seasonal_food(fruits, num_fruits, fruits_in_season, current_season);
    find_seasonal_food(veges, num_veges, veges_in_season, current_season);

    int i = 0;
    while (i < num_fruits) {
        if (fruits_in_season[i] == IN_SEASON) {
            num_fruits_in_season++;
        }
        i++;
    }

    int i = 0;
    while (i < num_veges) {
        if (veges_in_season[i] == IN_SEASON) {
            num_veges_in_season++;
        }
        i++;
    }

    int best_fruit = UNKNOWN;

    // Order the foods from best to worst
    sort_food(num_fruits_in_season, fruits_in_season);
    sort_food(num_veges_in_season, veges_in_season);

    // Find the fruit+vege that go the best together, so that our
    // dinner is as tasty as possible
    if (num_fruits_in_season > 0) {
        i = 0;
        while (i < num_fruits_in_season && best_fruit == UNKNOWN) {
            // Compare each of the fruits in season to the best vegetable
            if (great_combination(fruits_in_season[i], veges_in_season[0])) {
                best_fruit = fruits_in_season[i];
            }

            i++;
        }

        // If we didn't find any great combinations, just find a decent
        // combination
        if (best_fruit == UNKNOWN) {
            // If we only have one possible fruit, just pick that
            if (num_fruits_in_season == 1) {
                best_fruit = fruits_in_season[0];
            } else {
                i = 0;
                while (i < num_fruits_in_season && best_fruit == UNKNOWN) {
                    if (decent_combination(fruits_in_season[i], veges_in_season[0])) {

                        best_fruit = fruits_in_season[i];
                    }
                    i++;
                }
            }
        }
    }
    if (num_fruits_in_season == 0) {
        // If there aren't any fruits in season, we'll just have to eat
        // some imported fruits, bleh.
        best_fruit = choose_non_seasonal_fruit(num_fruits, fruits);
    }

    return best_fruit;
}

int great_combination(int fruit, int veg) {
    int great = FALSE;
    if (fruit * veg > 100000) {
        great = TRUE;
    }
    return great;
}

int decent_combination(int fruit, int veg) {
    return (fruit * veg > 10);
}

As I said at the start, this is a first attempt at moving some of the logic into functions.

In particular: the huge chunk of logic related to picking the best seasonal fruit and non-seasonal fruit have been moved to functions of thier own.

Several if statement conditions have been moved to functions of their own, to make the code more readable -- great_combination and decent_combination, rather than doing some confusing calculation within the if statement condition.

(note: I've shown two alternatives for how you might turn an if statement condition like that into a function -- either is fine, or there are other ways too).

Some #defines have also been added, to get rid of magic numbers.

This has made a substantial improvement to the code, but there's still even more that can be done.

Code moved to other functions, improved version.

Here's an even better attempt at cleaning up the code.

This version is broken up into even more, smaller functions.

Each function prototype (and function) has a good comment above it describing what it does.

Each function is kept simple.

There aren't any in-line comments in the code -- they're simply not necessary, because everything is moved into a well-named function, and the functions themselves are documented with what they're doing and why they're doing it.

#include <stdio.h>

#define SUMMER 0
#define AUTUMN 1
#define WINTER 2
#define SPRING 3

// Spring is the best season for fruits and veges
// (note: I made this up, I am not an expert on fruit or veges)
#define PREFERRED_SEASON SPRING

#define UNKNOWN -1
#define NOTHING -100

#define MAX_FOOD 1000

#define IN_SEASON 2

#define TRUE 1
#define FALSE 0

// Scans in "size" food items to the array.
void scan_food(int size, int array[]);

// Determine how good a combination of a certain fruit+veg is.
// Returns TRUE if it's a (great/decent) combination, and FALSE if it's
// not.
int great_combination(int fruit, int veg);
int decent_combination(int fruit, int veg);

// Chooses the best fruit to match the provided vegetable, given the
// fruit that are currently in season.
// Returns the fruit that was chosen.
int choose_best_seasonal_fruit(
    int num_fruits_in_season,
    int fruits_in_season[],
    int best_vegetable
);

// Chooses the best fruit to match the provided vegetable, without
// regard to season.
// Returns the fruit that was chosen.
int choose_best_non_seasonal_fruit(
    int num_fruit,
    int fruits[],
    int best_vegetable
);

// Determines what foods are currently in season, and copies them to the
// "seasonal" array.
// The "seasonal" array is ordered from best to worst.
// Returns the number of seasonal foods it put into the seasonal array.
int find_in_season_food(int food[], int num_fruits,
                       int seasonal[], int current_season);


// Sorts an array of foods from best to worst.
int sort_food(int size, int array[]);


// Choose the best fruit to eat, given the current season.
//
// This function handles getting all of the input from the user, and
// then calculates which fruit would be best to eat, when combined with
// the best vegetable.
//
// It takes into account the current season, and makes a decision based
// on that:
//   - Winter is the worst season, so don't eat any fruit.
//   - Spring is the preferred season, so if it's spring, choose a
//     fruit that's currently in season (if possible).
//   - For any other seasons, just choose the best fruit
//     (without taking into account the current season).

void choose_fruit_to_eat(void) {
    int num_fruits;
    int num_veges;
    int current_season;
    scanf("%d %d %d", &num_fruits, &num_veges, &current_season);

    int fruits[MAX_FOOD];
    scan_food(num_fruits, fruits);

    int veges[MAX_FOOD];
    scan_food(num_veges, veges);

    int fruits_in_season[MAX_FOOD];
    int num_fruits_in_season = find_in_season_food(
            fruits, num_fruits, fruits_in_season, current_season
    );

    int veges_in_season[MAX_FOOD];
    int num_veges_in_season = find_in_season_food(
            veges, num_veges, veges_in_season, current_season
    );

    int best_vegetable = veges_in_season[0];

    int best_fruit = UNKNOWN;

    if (current_season == WINTER) {
        best_fruit = NOTHING;
    }

    if (current_season == PREFERRED_SEASON) {
        best_fruit = choose_best_seasonal_fruit(
            num_fruits_in_season, fruits_in_season, best_vegetable
        );
    }

    if (best_fruit == UNKNOWN) {
        best_fruit = choose_best_non_seasonal_fruit(...);
    }

    print_chosen_fruit(best_fruit);
}

// Find the fruit that goes the best with the provided vegetable, so
// that our meal will be as delicious as possible.
//
// It first checks whether there are any great combinations.
//
// If it doesn't find any, it checks where there are any decent
// combinations.
//
// If there aren't any great or decent combinations, it returns
// "UNKNOWN", and the caller will need to use a different strategy to
// find a good fruit.
//
// Given that the fruits_in_season array is ordered from best to worst,
// it will always find the best possible fruit, if there are multiple
// "great" or "decent" combinations.
int choose_best_seasonal_fruit(
    int num_fruits_in_season,
    int fruits_in_season[],
    int best_vegetable
) {

    int best_fruit = UNKNOWN;

    int i = 0;
    while (i < num_fruits_in_season && best_fruit == UNKNOWN) {
        if (great_combination(fruits_in_season[i], best_vegetable)) {
            best_fruit = fruits_in_season[i];
        }

        i++;
    }

    if (best_fruit == UNKNOWN) {
        i = 0;
        while (i < num_fruits_in_season && best_fruit == UNKNOWN) {
            if (decent_combination(fruits_in_season[i], best_vegetable)) {
                best_fruit = fruits_in_season[i];
            }
            i++;
        }
    }

    // [comment for this blog post, shouldn't be left in submitted code]
    // Note that we didn't have to explicitly check whether there were
    // any fruits in season, because the loops won't execute if there
    // aren't (since 0 is not less than 0), so best_fruit will still be
    // UNKNOWN.

    return best_fruit;
}

int great_combination(int fruit, int veg) {
    int great = FALSE;
    if (fruit * veg > 100000) {
        great = TRUE;
    }
    return great;
}

int decent_combination(int fruit, int veg) {
    return (fruit * veg > 10);
}

If you look at the choose_best_seasonal_fruit function, there have been quite a few changes from the previous version:

All of the work to calculate and sort the seasonal foods has been moved to the parent function
There is a good, descriptive comment above the function which describes what it does, and thus the code doesn't need any comments within the function itself.
The function itself is very short and simple -- less than 20 lines of actual code.