Generating a string with only one space between the words

Question

I wrote a program that inputs a string with spaces between them and should allocate a new string with only 1 space between the words. I was wondering if there's a better way to write it (I rewrote it a couple of times and I'm trying to find the most effective way to write this code).

#define _CRT_SECURE_NO_WARNINGS
#define SIZE 80
#include <stdio.h>
#include <string.h>
#include <stdlib.h>

char* UpgradeString(char* oldText);
void inputStr(char* str);
int main()
{
    char str[SIZE], *ans;
    printf("Enter string\n");
    inputStr(str);

    ans = UpgradeString(str);
    puts(ans);

}
void inputStr(char* str)
{
    int i = 0;
    fgets(str, SIZE - 1, stdin);
    while (str[i] != '\n')i++;
    str[i] = 0;

}
char* UpgradeString(char* oldText)
{
    char temp[SIZE], *ans;
    int i=0, k = 0;

    while (1)
    {
        if (oldText[i] == 0)break;
        if (oldText[i] != ' ')
        {
            temp[k] = oldText[i];
            k++;
        }
        if (oldText[i] == ' '&&(oldText[i + 1] != ' '&&oldText[i + 1] != 0))
        {
            temp[k] = oldText[i];
            k++;
        }
        if (oldText[i] == ' ')i++;
        else i++;
    }
    temp[k] = 0;
    ans = (char*)malloc(sizeof(char)*(strlen(temp) + 1));
    strcpy(ans, temp);
    return ans;

}

Answer 1

Always specify length of array pointers.

In C, it is not always guaranteed that an array of char will be null terminated and therefore, the size can be determined by calling strlen. Always pass along the length of the array when passing a pointer to a function. This is not just for strings, but also for other pointer types which point to a dynamically allocated and static arrays.

Naming

Choose a consistent naming plan. The most commonly found style in most C code is snake_case. But whatever you choose, stick to it and don't deviate.

Const-correctness

If a method is not going to modify it's parameters, make that intention clear by accepting the arguments as const. This form of programming is what makes code self-documenting because you don't need to tell anyone that the function does not modify it's parameter, the const specifier tells the whole story.

Malloc + free

Always remember, for every malloc, there should be an equal and opposite free. Not freeing memory leads to unexpected memory leakages. This may not apply to you now, but just know this, many man hours have gone into developing tools for detecting memory leaks (valgrind, leakSanitizer, etc) when the best programmers are making extra effort not to leak memory, but still fail. Memory leaks are no joke. Free the memory when you are finished with it.

---

With all that being said, here is my rendition of your code

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <ctype.h> /* isspace */
#include <stddef.h> /* size_t */

#define SIZE (size_t)80

void inputStr(char*, size_t);
char* upgradeString(const char const*, size_t);

int main() {
    char string[SIZE] = {'\0'}, *result;
    printf("Enter string: ");
    inputStr(string, SIZE);

    size_t strLength = strlen(string);

    if (strLength > 0 && isspace(string[strLength - 1])) {
        string[--strLength] = '\0';
    }

    result = upgradeString(string, strLength);
    puts(result);
    free(result);
}

void inputStr(char* str, size_t length) {
    fgets(str, length, stdin);
}

char* upgradeString(const char const* oldText, size_t length) {

    size_t i, newSize = 0, foundSpace = 0;
    for (i = 0; i < length; i++) {
        if (isspace(oldText[i])) {
            if (foundSpace) {
                continue;
            }
            foundSpace = 1;
        } else {
            foundSpace = 0;
        }
        newSize++;
    }

    char *result = malloc((sizeof *result) * (newSize + 1));

    if (result) {
        char *temp = result;
        foundSpace = 0;
        for (i = 0; i < length; i++) {
            if (isspace(oldText[i])) {
                if (foundSpace) {
                    continue;
                }
                foundSpace = 1;
            } else {
                foundSpace = 0;
            }
            *temp++ = oldText[i];
        }
    }

    return result;
}

The changes made are based on the following additional considerations:

• What happens if the user enters nothing (standard input is empty)?
• What happens if the user just presses enter?

Conclusion

In conclusion be precise, be constant with naming, be explicit with intent. This will make your code robust and potentially bug free.

P.S.
_{It is not necessary to cast the result of malloc}

Answer 2

I think you've got the right idea here. You've separated your functionality out into different functions, which a lot of people forget to do. That makes it really easy to follow your code! I think you could make it a little simpler by using some standard library functions. Here are some thoughts I had while looking at this.

Simplify

In your UpgradeString() function, your loop is an infinite loop, and then you've got a condition inside that tells it to jump out of the loop. You can simplify this by just making the condition be the loop control like this:

while (oldText[i] != '\0')
{
    if (oldText [ i ] != ' ')
    { 
        // ...etc.

Use the Standard Library

You're doing a lot of work manually when you could just use the standard library to do it. The function strsep() will find the next occurrence of a string separator. strcat() will concatenate one string to another. You can use these to do less work. The one caveat is that strsep() modifies its input string. It's not my favorite interface, but it gets the job done. I solve the problem by duplicating the original string using strdup(). Here's how I would approach your UpgradeString() method:

char* UpgradeString(const char* oldText)
{
    char*   textCopy = strdup(oldText);   // Copy the original because strsep() modifies it
    char*   result  = calloc(SIZE, sizeof(char));
    if ((textCopy != NULL) && (result != NULL))
    {
        char*   nextStr = textCopy;
        while (textCopy != NULL)
        {
            nextStr = strsep(&textCopy, " "); // Get the location of the next space
            strcat(result, nextStr); // copy whatever was between the last space and this one into our result

            // Add a space after it if we're not at the end and we 
            // didn't find an empty string
            if ((nextStr != NULL) && (*nextStr != '\0') && (textCopy != NULL))
            {
                strcat(result, " ");
            }
        }
    }

    // Free the copy we made
    if (textCopy != NULL)
    {
        free(textCopy);
    }

    return result;
}