0

I have some code that completely passes check50, but prints results way too many times. For instance, here's a typical output:

~/pset3/plurality/ $ ./plurality Alice Bob Carol
Number of voters: 5
Vote: Alice
Vote: Carol
Vote: Bob
Vote: Carol
Vote: Alice
Carol
Alice
Carol
Alice
Carol
Alice
Carol
Alice
Carol
Alice

Two other examples:

~/pset3/plurality/ $ ./plurality Alice Bob Carol
Number of voters: 2
Vote: Bob
Vote: Bob
Bob
Bob
Bob
~/pset3/plurality/ $ ./plurality Alice Bob Carol
Number of voters: 1
Vote: Alice
Alice
Alice
Alice
Alice

Three results for 2, four results for 1... I don't get it at all.

Here's the code for the two functions I was supposed to change:

// Update vote totals given a new vote
bool vote(string name)
{
    for (int i = 0; i < candidate_count; i++)
    {
        if (strcmp(candidates[i].name, name) == 0)
        // 0 means both are the same in strcmp
        {
            candidates[i].votes++;
            return true;
        }
    }
    return false;
}

// Print the winner (or winners) of the election
void print_winner(void)
{
    // i leads to (candidate_count - 1) so I don't get a buffer overrun
    // accounts for the algorithim counting i+1 at the same time
    for (int i = 0; i < (candidate_count); i++)
    {
        if (candidates[i].votes > candidates[(i + 1)].votes)
        {
            // swaps the two variable positions
            int tempv = candidates[i].votes;
            string tempn = candidates[i].name;
            candidates[i].votes = candidates[(i + 1)].votes;
            candidates[i].name = candidates[(i + 1)].name;
            candidates[(i+1)].votes = tempv;
            candidates[(i+1)].name = tempn;
            // my intention is to restart the whole process every time I move a variable back
            // i think this recursion will make it loop back enough times to get everything
            print_winner();
        }
    }


    // print last candidate on list, line break, and then repeat for any other with same amount of votes
    for (int i = 0; i < (candidate_count); i++)
    {
        if (candidates[candidate_count].votes == candidates[(candidate_count - i)].votes)
        {
            printf("%s\n", candidates[(candidate_count - i)].name);
        }
        else
            break;
    }
}

I've tried to debug this and didn't understand what was happening. I'll put the code for the rest of the program below, although I believe I haven't changed this all. Thank you very much for your time.

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

// Max number of candidates
#define MAX 9

// Candidates have name and vote count
typedef struct
{
    string name;
    int votes;
}
candidate;

// Array of candidates
candidate candidates[MAX];

// Number of candidates
int candidate_count;

// Function prototypes
bool vote(string name);
void print_winner(void);

int main(int argc, string argv[])
{
    // Check for invalid usage
    if (argc < 2)
    {
        printf("Usage: plurality [candidate ...]\n");
        return 1;
    }

    // Populate array of candidates
    candidate_count = argc - 1;
    if (candidate_count > MAX)
    {
        printf("Maximum number of candidates is %i\n", MAX);
        return 2;
    }
    for (int i = 0; i < candidate_count; i++)
    {
        candidates[i].name = argv[i + 1];
        candidates[i].votes = 0;
    }

    int voter_count = get_int("Number of voters: ");

    // Loop over all voters
    for (int i = 0; i < voter_count; i++)
    {
        string name = get_string("Vote: ");

        // Check for invalid vote
        if (!vote(name))
        {
            printf("Invalid vote.\n");
        }
    }

    // Display winner of election
    print_winner();
}
1

The problem with using recursion here, is that it executes x times not only the part with the sorting but also the printing part. If you want to go with recursion you could split into two separate functions: the recursive one, for the sorting part, and other with the printing part.

If you want to keep it in one function, you could simply change the value of 'i' every time you swap a candidate...

| improve this answer | |
  • This looks correct; I think I hadn't realized that print_winner would keep going after it did that first loop. I don't 100% understand how changing the value of i would work outside of the loop I already have, either – dubflicker May 24 at 3:10
  • I meant change the value of 'i' after you do a swap, inside the first loop, to start over from the begining of the array. For example set the value to -1 since it'll increment by 1 in the next iteration. – Tritum May 24 at 6:25

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .