1

I've written a function that requires the user to enter a string of size 13, 15, or 16. The function keeps looping till the required length is entered. I am new to C. This function is part of one of the problem in havard CS50 online course. Though the function works, the logic is hard to follow. Can anyone please help me improve the code.

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

char *getString() // Returns pointer to a string. Input must be string with length 13, 15 or 16.
{
  short lengthOne = 13, lengthTwo = 15, maxLength = 16;
  char *str = (char *)malloc(sizeof(char) * maxLength + 1);

  do
  {
    printf("Enter a %d, %d, or %d-char string: ", lengthOne, lengthTwo, maxLength);
    fgets(str, maxLength + 2, stdin);

    for (int i = 0; i < strlen(str); i++)
    {
      if (str[i] == '\n')
        str[i] = '\0';
    }

    if (strlen(str) > maxLength)
    {
      while (getchar() != '\n')
        ;
    }

  } while (strlen(str) != maxLength && strlen(str) != lengthOne && strlen(str) != lengthTwo);

  return str;
}

int main()
{
  char *string = getString();
  printf("Content of string: %s\n", string);
  printf("Length of string is: %lu\n", strlen(string));
  return 0;
} 
Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278
kbl
  • 129
  • 5
  • 1
    You don't need a loop to replace the `\n`. It will always be the last character. – Barmar Jun 24 '22 at 18:57
  • 5
    Because you do: `malloc(sizeof(char) * maxLength + 1)`, then doing: `fgets(str, maxLength + 2, stdin);` is UB (undefined behavior). Because you're letting `fgets` go one byte past the end of `str`. You want the same length passed to `malloc`, which is `maxLength + 1`. – Craig Estey Jun 24 '22 at 18:58
  • I understand that fgets places the null terminator at the end of strings but I need the loop to get rid of the newline character – kbl Jun 24 '22 at 18:59
  • Please see [Removing trailing newline character from fgets() input](https://stackoverflow.com/questions/2693776/removing-trailing-newline-character-from-fgets-input/28462221#28462221) – Weather Vane Jun 24 '22 at 19:04
  • 1
    A much easier way of implementing this sort of thing is: (1) allocate array of size 100 or so. (2) read line from user. (3) strip newline if necessary. (4) use `strlen` to compute length of line. (5) if length is not as required, complain. – Steve Summit Jun 24 '22 at 19:07
  • An important rule in C, not nearly as well known as it should be, is: "If you are trying to read a string of length N from the user, do *not* try to use an array of size N (or of size N+1) to hold the user's response." – Steve Summit Jun 24 '22 at 19:08
  • @Steve Summit. Your advise seems easier at my first glance. I'll try to code using it, then I'll get back to you. Thanks. – kbl Jun 24 '22 at 19:13
  • 2
    A major mistake in this code is the repeated use of `strlen` in your loops tests. This turns what should be O(n) loops into O(n^2) loops. Every time you call `strlen`, it scans the *entire* string to determine its length. Why would you do that more than once for a given string? If you were doing it by hand, you wouldn't. Properly written code should be no different. – Tom Karzes Jun 24 '22 at 20:02
  • @Tom Karzes. You're absolutely right. There's no reason to call a function on an array that produces the same length each time. Just save the length in a variable. Thanks! I am still learning :) – kbl Jun 24 '22 at 20:14
  • 1
    In your `for` loop you can replace `i < strlen(str)` with `str[i] != 0` – Craig Estey Jun 24 '22 at 20:27

1 Answers1

0

I recently asked for help with getting string input with specified length. The advices I received were very helpful. Thanks to those took the time to comment. I rewrote the function with the little new understanding I attained. Of course, I think this function works fine, just as I did with the previous erroneous function I wrote :). Please give me pointers if there are improvements that could be made to my code. Thanks!

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

char *getString() // Returns pointer to a string. Input must be string of exact length 13, 15 or 16.
{
    short lengthOne = 13, lengthTwo = 15, lengthThree = 16, maxLength = 17; // setting maxLength = 16 causes return value
                                                                            // to be user's first 16 chars, not desired result.
    short finalLength;
    char *string = (char *)malloc(sizeof(char) * maxLength + 1);

    do
    {
        printf("Enter string of size %d, %d, or %d: ", lengthOne, lengthTwo, lengthThree);
        fgets(string, maxLength + 1, stdin);

        short initialLength = strlen(string);

        if (initialLength == maxLength && string[initialLength - 1] != '\n')
        {
            while (getchar() != '\n')
                ;
        }

        for (int i = 0; string[i] != '\0'; i++)
        {
            if (string[i] == '\n')
                string[i] = '\0';
        }

        finalLength = strlen(string);

    } while (finalLength != lengthOne && finalLength != lengthTwo && finalLength != lengthThree);

    return string;
}

int main()
{
    char *string = getString();
    printf("content of string is: %s\n", string);
    printf("length of string is: %lu\n", strlen(string));

    return 0;
}

 
kbl
  • 129
  • 5