No more filthy words

Question

Challenge

Given a list of words mixed with extra symbols. Write a program that will clean up the words from extra numbers and symbols.

Specifications

1. The first argument is a path to a file.
2. Each line includes a test case.
3. Each test case is a list of words.
4. Letters are both lowercase and uppercase, and mixed with extra symbols.
5. Print the words separated by spaces in lowercase letters.

Constraints

1. The length of a test case together with extra symbols can be in a range from 10 to 100 symbols.
2. The number of test cases is 40.

Input Sample

(--9Hello----World...--)
Can 0$9 ---you~
13What213are;11you-123+138doing7

Output Sample

hello world
can you
what are you doing

Source

Solution:

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

void to_lowercase(char * input) {
    for(int i = 0; input[i]; i++){
        input[i] = tolower(input[i]);
    }
}

char * sanitize(char * input) {
    char *sanitized = malloc(sizeof (char) * 1024);
    int iterator = 0;
    int character_value;
    bool wordMatched = false;

    for (int i = 0; i < strlen(input); i++) {
        int character_value = input[i];
        // a-z = 97-122, A-Z = 65-90
        if (character_value >= 97 && character_value <= 122
        ||  character_value >= 65 && character_value <= 90) {
            sanitized[iterator++] = input[i];
            wordMatched = true;
        } else if (wordMatched) {
            wordMatched = false;
            sanitized[iterator++] = ' ';
        }
    }

    sanitized[iterator] = '\0';
    to_lowercase(sanitized);
    return sanitized;
}

int main(int argc, const char * argv[]) {
    FILE *file = fopen(argv[1], "r");
    char line[1024];
    while (fgets(line, 1024, file)) {
        printf("%s\n", sanitize(line));
    }

    return 0;
}

Answer 1

Look here:

for (int i = 0; i < strlen(input); i++) {

Note that strlen(input) is O(n), proportional to the length of the input. That makes your algorithm O(n²), which is slower than it should be. If you need to call strlen(), make sure to call it just once. However, this problem is easily solvable without using strlen() at all.

---

Others have pointed out your memory leak. What should be done about it? The output is never longer than the input, right? Therefore, I would say that your best option is to overwrite the input with the sanitized output. There is no need to allocate any additional memory, and no need to worry about the buffer size. (If the caller wants to keep the original string, then the caller can duplicate it first.)

There is no need to write a to_lowercase() function that calls tolower() on each character in the string. Just call tolower() as part of the loop.

The character_value comparisons could be simplified to isalpha(character_value). Your naming style is inconsistent between character_value and wordMatched. I would change int character_value to char c.

---

With this code…

} else if (wordMatched) {
    wordMatched = false;
    sanitized[iterator++] = ' ';
}

… you output a space when transitioning from a letter to a non-letter. However, that would output a space corresponding to the 7 at the end of 13What213are;11you-123+138doing7. In my opinion, it would be better if it didn't output a space at the end.

Suggested solution

I've put some miscellaneous remarks in comments.

#include <assert.h>
#include <ctype.h>
#include <stdbool.h>
#include <stdio.h>              // You missed this for printf(3)
#include <stdlib.h>

/**
 * Replaces consecutive non-alphabetic characters in the input
 * string with a single space.  Non-alphabetic characters at the
 * beginning and end are trimmed off as well.  The remaining ASCII
 * letters are replaced with their lowercase counterparts.
 *
 * The input string will be overwritten.
 *
 * Returns the length of the sanitized output.
 */
size_t sanitize(char *s) {
    bool needSpace = false;
    char *out = s;
    for (char *in = s; *in != '\0'; in++) {
        assert(out <= in);
        if (isalpha(*in)) {
            if (needSpace) *out++ = ' ';
            needSpace = false;
            *out++ = tolower(*in);
        } else if (out > s) {
            needSpace = true;
        }
    }
    *out = '\0';
    return out - s;
}

int main(int argc, const char *argv[]) {
    char line[1024];
    FILE *file = stdin;        // Read from stdin if no filename given
    if (argc > 1 && !(file = fopen(argv[1], "r"))) {
        perror(argv[1]);       // Some error handling that you didn't have
        return EXIT_FAILURE;
    }
    while (fgets(line, sizeof(line), file)) {
        sanitize(line);
        puts(line);            // Don't need printf() when puts() will do
    }
}

Answer 2

In sanitize, you're allocating memory which you aren't explicitly freeing:

char *sanitized = malloc(sizeof (char) * 1024);

In your small program, this will be cleaned up when the program exits, however you still want to get into the habit of freeing memory that you're allocating. If you were to reuse the sanitize method in a longer lasting program you may well end up crashing the program with an out of memory error. One way to do this would be to free the memory from your main method:

while (fgets(line, 1024, file)) {
    char *sanitized = sanitize(line)
    printf("%s\n", sanitized);
    free(sanitized);
}

Alternately, you could rewrite the line that is being sanitized in place, rather than creating a new destination to copy the sanitized input into, or pass in a second buffer to receive the output so you would end up with a function signature like this:

void sanitize(char * input, char *destination, int destinationSize)

Used like so:

char sanitized[1024];
while (fgets(line, 1024, file)) {
    sanitize(line, sanitized, 1024)
    printf("%s\n", sanitized);
}

At which point you might want to start thinking about using a define for buffer size, rather than having the 1024 magic number everywhere...

Answer 3

        // a-z = 97-122, A-Z = 65-90
        if (character_value >= 97 && character_value <= 122
        ||  character_value >= 65 && character_value <= 90) {

I'd find this easier to read as

        if (character_value >= 'a' && character_value <= 'z'
         || character_value >= 'A' && character_value <= 'Z') {

From a code perspective it won't matter. Both will produce the same code. But from a readability perspective, I'd find this easier to follow. It also helps in that it eliminates the need for the comment, making the code more maintainable (since comment and code can no longer fall out of sync).

I'd also prefer to see 1024 as a constant. You use it in three separate places, one of them rather removed from the other two. Another trick you can use is to pass the size into sanitize:

char * sanitize(char * input, size_t size) {
    char *sanitized = malloc(size);

which you'd use as sanitize(line, sizeof line). Applying sizeof line to the latter two places where you use 1024, you can get down to just the use in the declaration.

Answer 4

^{Comments transferred into an answer.}

Other answers have given an analysis of the major problems. This gives some information that is also of some importance.

You don't error check the value from fopen(). You don't check that the program was given an argv[1] to work with (it could be a null pointer). You don't check the return from malloc(). Given the size constraint of "maximum 100", allocating 1024 bytes seems like overkill. (In fact, it would be simpler to use a fixed size allocation and avoid using malloc().)

The maximum was altered, I'm unused to allocating memory so I wasn't sure if it was in bits or bytes. As for the error-handling, while I agree in general the environment is focused on the challenge and has guaranteed input. Otherwise, I'd need the same boiler-plate error-handling for each challenge. But I could be mistaken in that it justifies the omission.

In a constrained environment, it may be OK to omit error checks for argv[1] etc, but it doesn't take much code and is necessary in 'the real world'. Not checking malloc() et al is living dangerously. Again, you may get away with it in the constrained environment, but you simply have to deal with problems somehow — even if it means reporting an error and exiting under control, that is better than crashing because it failed and the code didn't check.

The only place you specify bits in C is in a bit-field. They're arcane and not very portable (almost all implementation-defined behaviour). In other words, with malloc() you're requesting bytes.