5
\$\begingroup\$

I'm fairly new to C and wanted a general review of this code. It's a solution to this problem.

You and your \$K-1\$ friends want to buy \$N\$ flowers. Flower number \$i\$ has cost \$c_i\$. Unfortunately the seller does not want just one customer to buy a lot of flowers, so he tries to change the price of flowers for customers who have already bought some flowers. More precisely, if a customer has already bought \$x\$ flowers, he should pay \$(x+1)*c_i\$ dollars to buy flower number \$i\$.

You and your \$K-1\$ friends want to buy all \$N\$ flowers in such a way that you spend the least amount of money. You can buy the flowers in any order.

Input:

The first line of input contains two integers \$N\$ and \$K\$ (\$K \le N\$). The next line contains \$N\$ space separated positive integers \$c_1,c_2,\dotsc,c_N\$.

Output:

Print the minimum amount of money you (and your friends) have to pay in order to buy all \$N\$ flowers.

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

int comp_desc(const void * a, const void * b)
{
    int * aPtr = (int*)a;
    int * bPtr = (int*)b;
    return *bPtr - *aPtr;
}

int main()
{
    int flowersNeeded, numFriends, i;
    scanf("%d %d", &flowersNeeded, &numFriends);
    getchar();
    int flowerCosts[100];
    memset(flowerCosts, 0, sizeof(flowerCosts));
    for (i = 0; i < flowersNeeded; ++i)
    {
        scanf("%d", &flowerCosts[i]);
    }
    qsort(flowerCosts, flowersNeeded, sizeof(int), comp_desc);
    int flowersBought = 0;
    int moneySpent = 0;
    int multiplier = 1;
    for (i = 0; i < flowersNeeded; ++i)
    {
        moneySpent += flowerCosts[i] * multiplier;
        multiplier = (++flowersBought / numFriends) + 1;
    }
    printf("%d\n", moneySpent);
    return 0;
}
\$\endgroup\$
1
\$\begingroup\$

It's maybe unnecessary for this little code, but I'd get into the habit of writing different functions for the different steps of your algorithm. You'd notice that breaking it in smaller functions gives readability, and the opportunity to expand the functionality easily.

For example, I broke your code to smaller functions, which let me add some error checking (removing the "pedantic" warnings mentioned by @Caridorc, checking that number of flowers is not greater than the array size, ...). Adding error checking would have been much more difficult (harder to read) if it had been done directly in the main function.

Writing smaller function also makes the code more self-documenting: by choosing good function names, you're really saying what the code is doing now.


Initializing to 0 with memset is a bit old school. You can 0-init arrays this way:

int array[A_GOOD_DEFINE] = {0};

Defensive programming (as mentioned by @Caridorc) is very valuable. Always use return values (look up perror). It's more code, most of your function calls are made as if statements, but it's really worth it, a very good habit to get into. I forgot how to use a debugger since I starting coding that way.

My example of user_input_get() below looks a bit silly, partly because of the bracing style (it's only an opinion, but since defensive programming is doing nearly everything in if statements, there are a lot of rows with only braces. Especially if you don't want to have braceless if bodies). To solve this it could be broken down even further: user_input_sizes_get() and user_input_costs_get(). Left for you to do :)


Prompt the user for input with printf, and what should be entered next. Right now the program just waits without asking anything, it looks like it froze.


With these changes applied (again, break the input function further). I believe the main function shows a clearer overview.

#include <errno.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>

#define COSTS_ARRAY_SIZE    (100u)

int comp_desc(const void * a, const void * b)
{
    int * aPtr = (int*)a;
    int * bPtr = (int*)b;
    return *bPtr - *aPtr;
}

int user_input_get(int *needed, int *numFriends, int *costs, size_t size)
{
    errno = 0;
    if (printf("Enter number of needed flowers, then number of friends: ") < 0)
    {
        perror("printf");
        return -1;
    }

    int n_read_values = scanf("%d %d", needed, numFriends);
    // The following inspired by the man page for scanf.
    if (n_read_values != 2)
    {
        if (errno != 0)
        {
            perror("scanf");
        }
        else
        {
            fprintf(stderr, "No matching characters\n");
        }
        return -1;
    }
    else
    {
        if (*needed > COSTS_ARRAY_SIZE)
        {
            fprintf(stderr, "Too many flowers needed\n");
            return -1;
        }

        for (int i = 0; i < *needed; ++i)
        {
            if (printf("Cost of flower %d: ", i + 1) < 0)
            {
                perror("printf");
                return -1;
            }
            n_read_values = scanf("%d", &costs[i]);
            if (n_read_values != 1)
            {
                if (errno != 0)
                {
                    perror("scanf");
                }
                else
                {
                    fprintf(stderr, "No matching characters\n");
                }
                return -1;
            }
        }
    }
    return 0;
}

int money_spent_get(int needed, int costs[], int numFriends)
{
    int spent = 0;
    int bought = 0;
    int multiplier = 1;

    for (int i = 0; i < needed; ++i)
    {
        spent += costs[i] * multiplier;
        ++bought;
        multiplier = (bought / numFriends) + 1;
    }
    return spent;
}

int main()
{
    int flowersNeeded, numFriends;
    int flowerCosts[COSTS_ARRAY_SIZE] = {0};     // Better init than memset()

    if (user_input_get(&flowersNeeded, &numFriends, flowerCosts, COSTS_ARRAY_SIZE) < 0)
    {
        fprintf(stderr, "Error: user_input_get\n");
        exit(EXIT_FAILURE);
    }

    qsort(flowerCosts, flowersNeeded, sizeof(int), comp_desc);

    int moneySpent = money_spent_get(flowersNeeded, flowerCosts, numFriends);

    printf("%d\n", moneySpent);
    exit(EXIT_SUCCESS);
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for the advice, everything looks great except this program was to solve a programming challenge (see the link in the question), so if you print text other than the answer you will get "wrong answer". \$\endgroup\$ – reggaeguitar Jun 10 '15 at 16:39
  • \$\begingroup\$ I understand, it's for automated testing! \$\endgroup\$ – Gauthier Jun 11 '15 at 11:27
3
\$\begingroup\$

You should declare loop variables as close as possible to the loop

int flowersNeeded, numFriends, i;
// ... some code
for (i = 0; i < flowersNeeded; ++i)

The above code causes inefficiency and puts unecessary mental burden on the reader, you should use (in C99):

int flowersNeeded, numFriends;
// ... some code
for (int i = 0; i < flowersNeeded; ++i)

Avoid modifyng in place and assigning at the same time

multiplier = (++flowersBought / numFriends) + 1;

The above line is too much compact, usually you assign to a variable xor you increment a variable in place, I would explode it in two lines:

flowersBought++;
multiplier = (flowersBought / numFriends) + 1;

Sorry for being pedantic

The very same compiler you use to generate the binaries doubles as a code analysis tool:

gcc flowers.c -Wall -pedantic 

Warns you about:

h.c: In function ‘main’:
h.c:15:5: warning: ignoring return value of ‘scanf’, declared with attribute warn_unused_result [-Wunused-result]
     scanf("%d %d", &flowersNeeded, &numFriends);
     ^
h.c:21:9: warning: ignoring return value of ‘scanf’, declared with attribute warn_unused_result [-Wunused-result]
         scanf("%d", &flowerCosts[i]);
         ^
\$\endgroup\$
3
\$\begingroup\$

About this code:

scanf("%d %d", &flowersNeeded, &numFriends);
getchar();
int flowerCosts[100];
memset(flowerCosts, 0, sizeof(flowerCosts));
for (i = 0; i < flowersNeeded; ++i)
{
    scanf("%d", &flowerCosts[i]);
}
  • What's the purpose of that getchar() there?
  • Why memset for flowerCosts, if you're overwriting the values anyway?

You could simplify the above code to this:

scanf("%d %d", &flowersNeeded, &numFriends);
int flowerCosts[100];
for (i = 0; i < flowersNeeded; ++i)
{
    scanf("%d", &flowerCosts[i]);
}

In comp_desc, although the function receives pointers as parameters, the main logic in the implementation doesn't need pointers, it needs two integers. So instead of converting the (void*) to (int*), the implementation would be slightly cleaner if you extracted the int values directly, so that the main logic can focus on the main purpose, without doing pointer arithmetics, like this:

int comp_desc(const void * aPtr, const void * bPtr)
{
    int a = *(int*)aPtr;
    int b = *(int*)bPtr;
    return b - a;
}
\$\endgroup\$
  • \$\begingroup\$ You can't set an array to a dynamic number of elements without using malloc, your code won't compile because of the line int flowerCosts[flowersNeeded] \$\endgroup\$ – reggaeguitar Jun 3 '15 at 20:46
  • \$\begingroup\$ You're right. I dropped that point. \$\endgroup\$ – janos Jun 3 '15 at 21:05
  • 1
    \$\begingroup\$ Technically variable length arrays are legal C99 code (although it was dropped back to being only optionally supported in C11). Using malloc will definitely be more portable, however. \$\endgroup\$ – Yuushi Jun 3 '15 at 21:07

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.