0

I am going through the problem set 3 and I am currently stuck at the record preference (vote) function. I have built a code down as below, but running it gives me a segmentation fault error message. Any advice? Thanks.

Here's my code:

// Record preference if vote is valid
bool vote(int voter, int rank, string name)
{
    // TODO
    // Assume that the name is not yet given and update if a name is given
    bool name_exist = false;

    //Look for a candidate called name
    for (int i = 0; i <= candidate_count ; i++)
    {
        //If the input matches a candidate list update the preference
        {
            for (int j = 0; j < i ; j++)
            {
                if (strcmp(candidates[i].name, name) == 0)
                {
                    preferences[voter][rank] = candidates[i].votes;
                    printf("%s\n",name);
                }

            }
            name_exist = true;
        }
    }
    return name_exist;
1

I don't see the need for two loops. The inner one does not seem to serve any purpose (j is not used, the check inside should be performed exactly once per value of i). The outer one should probably be i < candidate_count instead of <= (this is likely your segmentation fault, interpreting candiates[candidate_count] as a string).

The value assigned to preferences[voter][rank] should be the candidate index i, not their number of votes (which doesn't even contain any useful value at that point).

| improve this answer | |
  • Yeah, I think I kind of get what you mean. I'll try and fix my code. Thanks @Blauelf! – speedy_catch Jan 22 at 3:51
  • Hey @speedy_catch did you manage to make your code work? I wrote preferences[i][j] = candidate[i]; to store the candidate index as the preference but I have an error message :-( – Melsunshine Jan 25 at 23:27
  • @Melsunshine You should assign the index (i), not the candidate struct candidates[i]. Also, preferences[voter][rank], you need only one loop, the other variables are already given. – Blauelf Jan 25 at 23:37
  • @Blauelf I get it! I made this too complicated for nothing, thanks – Melsunshine Jan 28 at 1:11
1

There are a couple of things I would change in the code based on good coding practices and design.

Firstly, you don't need to create the variable:

bool name_exist = false;

It uses up memory when you never needed it. The best practice would be at the end of the loop return true, instead of changing a variable. This also means that you're not changing the last part of the function as it was correct: return false;. Once your 'return' you break out of the function and continue from where the function was called. You want to return as soon as possible instead of running through the entire function, only increasing the time to run for no extra work.

I would also suggest that instead of:

if (strcmp(candidates[i].name, name) == 0)

You could simply write:

if (!strcmp(candidates[i].name, name))

Typing in a !x is the same as x == 0. This isn't a big deal but '!' is useful and it's good practice to using it.

I never did this problem; I had done tideman instead, so I can't help you further, but good Luck!

| improve this answer | |

You must log in to answer this question.

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