3
\$\begingroup\$

This program is used to get names and numbers of N person, print out the numbers of a person. Please help me improve this code.

Additional Question:

To avoid a very long name that exceeds char's size, I limit it to 255. Is there a better way?

To avoid buffer, I included a space before % sign when using scanf and not use getchar(). Is there a better way?

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

// Limiting string input
const unsigned int STRINGCONSTRAINT = 255;

// Person struct to store name and numbers
typedef struct {
  char name[STRINGCONSTRAINT];
  char numbers[STRINGCONSTRAINT];
} person;

void getNameNumber(person people[], int size);
void printNumber(person people[], int size, char personFind[]);

// Asked for array's size and a person's name to find
int main(void) {
  unsigned int size = 0;
  printf("Enter the amount of people: ");
  scanf(" %d", &size);

  person people[size];
  getNameNumber(people, size);

  char personFind[STRINGCONSTRAINT];
  puts("Who do you want to find?");
  printf("Person to find: ");
  scanf(" %[^\n]s", personFind);

  printNumber(people, size, personFind);

  return 0;
}

// Get name and numbers for the array
void getNameNumber(person people[], int size) {
  unsigned int i;
  for (i = 0; i < size; i++) {
    printf("Name: ");
    scanf(" %[^\n]s", people[i].name);
    printf("Numbers: ");
    scanf(" %[^\n]s", people[i].numbers);
  }
}

// Find the person and print the numbers
void printNumber(person people[], int size, char personFind[]) {
  unsigned int i;
  int flag = 1;

  for (i = 0; i < size; i++) {
    flag = strcmp(people[i].name, personFind);
    if (flag == 0) {
      puts("Found!");
      printf("You can call %s at %s.\n", personFind, people[i].numbers);
    }
  }
}
\$\endgroup\$

3 Answers 3

3
\$\begingroup\$

static functions

Both of your getNameNumber and printNumber should be made static, because I doubt that you plan to export them.

Simpler functions

printf("Enter the amount of people: ");

can simply use puts, which doesn't have any formatting code in it. You use puts below, so this would also help with consistency.

Stack allocation

person people[size];

is not available in all versions of C, since size is not a constant. For best portability you'd want to malloc this, or perhaps alloca if you feel strongly that the memory should be in the stack and not on the heap.

const

printNumber's array arguments should be const, since you don't mutate them.

Your questions

To avoid a very long name that exceeds char's size, I limit it to 255. Is there a better way?

For your use case, not really. A fixed array is the most appropriate. However, your code is vulnerable to overflows. You should be putting a width in your format specifier.

To avoid buffer, I included a space before % sign and not use getchar(). Is there a better way?

I'm not sure which usage of % you're referring to, but they all look fairly sane, so no; there's not really a better way.

\$\endgroup\$
3
  • \$\begingroup\$ "is not available in all versions of C, since size is not a constant". VLA available in C99 and since C11 compiler might not support VLA (in this case macro constant __STDC_NO_VLA__ equals 1). \$\endgroup\$
    – eanmos
    Oct 16, 2019 at 4:22
  • 1
    \$\begingroup\$ Wasn't puts the function that adds a newline, in contrast to fputs? Therefore it is not a good replacement for this printf call. \$\endgroup\$ Oct 16, 2019 at 4:24
  • \$\begingroup\$ "can simply use puts" --> not same functionality. \$\endgroup\$ Oct 26, 2019 at 9:38
1
\$\begingroup\$

Strange format

The s in scanf(" %[^\n]s", personFind) serves no purpose. Use " %[^\n]" instead

... or better, use a width limit. See scanf / field lengths

\$\endgroup\$
1
\$\begingroup\$

Consider this line:

  scanf(" %d", &size);

We throw away the return value from this function, which means we have no way of knowing whether a value was written to size. In this case, we don't have undefined behaviour, because we initialised it to zero before calling scanf(), but that's not the case later in the program where we read strings into uninitialized memory. In any case, it's rude to users to silently change invalid input into a default answer, rather than giving a meaningful error message.

BTW, the space at the beginning of that conversion is redundant - numeric conversions always skip leading whitespace.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.