Trying to write a function that get names from the user

Question

I tried to write a function, that get a number of candidates betwen 10 to 60,000, and gets a name for each candidate... This is what I wrote:

/********** Headers **********/
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <conio.h>
/********** Consts **********/
const number_candidates;

/********** Functions **********/
void get_candidates(char *arr[256][256])
{
    int counter = 1, i = 0, j =0;
    int temp = 0;

    printf ("Please enter the number of candidates: \n");
    scanf ("%d", &temp);
    while (temp < 10 && temp > 60000)
    {
        printf ("Please enter the number of candidates: \n");
        scanf ("%d", temp);
    }
    const number_candidates = temp;
    while (counter < number_candidates + 1)
    {
        printf ("Please enter the %d name: \n", counter);
        fgets (arr, 256, stdin);
        counter++;
    }
}

int main(int argc, char *argv[])
{
    int i = 0;
    char can_names[256][256];

    get_candidates(&can_names);

    system("PAUSE");
    return 0;
}

There is an error in getting the names into the arr...

Answer 1

Don't know if this would help but have a look at the code below. It works dynamically ie. it allocated memory for desired no of candidates rather than assuming 60,000 of them.

/*
 * Write a function, that get a number of candidates betwen 10 to 60,000, and gets a     name for each candidate 
 */

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define MAX 256

int main(int argc, char*argv[]){

int i,n;
char **s;

printf("Enter the total number of candidates\n");
scanf("%d",&n);

//error condition
if(n<10 || n>60000){        
    printf("Sorry number of candidates should be between 10 to 60,000\n");
    return -1;      
}

//allocate memory
s = malloc(sizeof(char*)*n);

//get the data
for(i=0;i<n;i++){

    s[i] = calloc(MAX,sizeof(char));
    printf("Enter the candidate number %d's name:\n",i+1);
    //fgets(s[i],MAX,stdin);
    scanf("%s",s[i]);

}

//Display the data
printf("\nDetails of all the Candidates\n\n");
for(i=0;i<n;i++){
    printf("Candidate number %d's name:%s\n",i+1,s[i]);
}

//Free the memory
for(i=0;i<n;i++){
    free(s[i]);
}
free(s);

return 0;

}

I had a problem with fgets it was skipping the first candidate info. Any help would be appreciated.. I tried flush(stdin) but did not solve the issue.

Answer 2

A few things are wrong:

First, you need space for 60000 names, yet you only allocate enough for 256. OK, we change

char can_names[256][256];

to

char can_names[60000][256];

and get... a segmentation fault, probably. That's because the array is using too much stack space. Change it to

static char can_names[60000][256];

so it's not on the stack.

---

Second, there's no need to take the address of the array - it's already passed as a pointer. Your function call changes to

get_candidates(can_names);

and the function signature is

void get_candidates(char arr[60000][256])

---

Third, you need a loop to read the entries one at a time. A for loop is easier to read:

for (counter = 0; counter < number_candidates; counter++)
{
    printf ("Please enter the %d name: \n", counter);
    fgets (arr[counter], 256, stdin);
}

---

Fourth, the condition

while (temp < 10 && temp > 60000)

should be

while (temp < 10 || temp > 60000)

(how can a number be both less than 10 and greater than 60000?) Once this is fixed, you can remove the initial read of temp since the loop will run at least once. Note that if you type a letter instead of a number now, the program will go into an infinite loop (it will repeatedly read the letter). Fixing this is left as an exercise.

---

Fifth, you don't need any headers except stdio.h. Also, the i and j variables are unused.

---

Edit: missed the scanf error. scanf takes addresses as parameters. it makes sense too: scanf needs somewhere to store a value, it doesn't care about the current value. So the call to scanf should be:

scanf ("%d", &temp);

Answer 3

You should avoid using arguments like this one: char *arr[256][256] ... what's the point of it? You should think about what your function will do. You want it to load names of candidates right? So you could define struct candidate with an attribute name within it:

typedef struct candidate{
    char name[256];
} Candidate;

Another thing: why are you passing an address of your array to this function? You just want your array to be filled with data, you won't work with an array itself, thus it's enough to pass an array, not an address of it.

Then prototype of your function could be changed to void get_candidates(Candidate* candidates) which is much easier to read. And look how simple can usage of this function become:

Candidate candidates[256];
get_candidates(candidates);

And last thing: before you write function like that, try something simpler first (to find out what's happening there). Here's an example:

#include <stdio.h>

typedef struct candidate{
    char name[256];
} Candidate;

void get_candidates(Candidate* candidates){
    scanf("%255s", candidates[4].name);
}

int main(int argc, char *argv[]){
    Candidate candidates[256];
    get_candidates(candidates);
    printf("%s\n", candidates[4].name);
    return 0;
}

In case you don't know the count of candidates before calling get_candidates, then it's better to change the prototype of this function to Candidate* get_candidates() so that it's clear that this function creates an array:

// caller is responsible for calling free on return value
Candidate* get_candidates(){
    Candidate* candidates;
    int count = 50; // here you found out the count
    candidates = malloc(count*sizeof(Candidate));
    fgets(candidates[4].name, 255, stdin);
    return candidates;
}

int main(int argc, char *argv[]){
    Candidate* candidates = get_candidates();
    printf("%s\n", candidates[4].name);
    free(candidates);
    return 0;
}

Answer 4

Have a look at the documentation for scanf which indicates that variables need to be passed as a pointer as you did the first time you call scanf. Then have a look at your second call to scanf...

You're currently only assigning names to the first string in your array over and over again. Look at that while loop and in particular, how you're passing in the 'arr' variable. Have a look here for some inspiration.

To print out all of the names you need to loop over the array. You can find some examples of printing a list of strings here and here.

Answer 5

You should call:

counter = 0;
...
fgets (arr[counter], 256, stdin);

You need walk one step for each loop.