HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Algorithm for Run Length Encoding - String Compression

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
lengthcompressionalgorithmforencodingstringrun

Problem

I attempted a problem from the Cracking the Coding Interview book. The following input: aabcccaaa should give the following output: a2b1c3a3. Any advice is much appreciated!

#include 
#include 
#include 

char* compress(char*);

int main(int argc, char** argv)
{   
    if(argc==2)
    {
        printf("The compression of %s is %s \n", argv[1],
            compress(argv[1]));
    }
    else
        printf("Not correct number of inputs.");
}

char* compress(char* str)
{
    const size_t len = strlen(str);
    char result[len];

    //initialized to 0, so length check can be done later
    memset(result, 0, len); 

    int counter;
    int j = 0;

    if(str==NULL || len == 0)
        return 0; 

    result[0] = str[0];
    for(size_t i = 0; i < len; i++)
    {   
        counter = 1;//reset counter

        //to make sure no array out of bounds exception occurs
        if(result[j+1]==0)
            result[++j] = counter + '0';
        else
            return str;

        while(str[i] == str[i+1])
        {
            //to store number in char array, add the asci value 
            // of 0.
            result[j] = (++counter + '0'); 
            i++;
        }
        if(result[j+1]==0)
            result[++j] = str[i+1];
        else 
            return str;
    }

    result[++j] = '0';

    //to avoid "function returns address of local variable" error
    //to also avoid altering original argument
    char* str1 = (char*)malloc(sizeof(result));
    strcpy(str1, result);
    return str1;
}

Solution

-
You can eliminate that function prototype by defining main() last, but it's up to you.

-
It's more common to first check the command line usage and then exit with a non-zero value if it's invalid. You should also print all errors to stderr, not stdout (the latter is always used with printf()).

if (argc != 2)
{
    fprintf(stderr, "Not correct number of inputs.");
    return -1;
}


-
You can probably check str right at the start, especially with the len assignment. If there is an error, then it may be clearer to return NULL instead of 0. At least that's what many library functions return specifically.

-
The while loop can instead be a for loop:

for (; str[i] == str[i+1]; i++)
{
    //to store number in char array, add the asci value 
    // of 0.
    result[j] = (++counter + '0');
}


-
The NULL character for result is wrong; it should be '\0'.

-
There's actually no need to cast the return type of malloc() in C. Although malloc() does return a void, it's not required because any pointer type can be assigned to a void and is done "automatically" here.

Moreover, you can avoid that aforementioned error by allocating result after making sure that the original string is valid. This will also avoid the need to call strcpy() at the end and risk other bugs. Since result will not be local this way, you can return it from the function without losing the data.

char* compress(char* str)
{
    // check to make sure str is valid...

    // assuming len is also valid
    char* result = malloc(len);

    // OR, use calloc() instead to get the memory zeroed
    //   thus no need for memset() or anything else
    char* result = calloc(len, sizeof(char));

    // probably also good to check for sufficient memory
    // if insufficient, print error and abort
    if (!result)
    {
        perror("malloc"); // or "calloc"
        abort();
    }

    // do the compression...

    // return it right away; no need to copy anything
    return result;
}


-
You never call free() on str1, so this function leaks memory. You would have to do this in main() with the returned string, and it may be necessary to assign compress() to a const char to display before freeing afterward. This also means that compress() should return a const char, not a char*, preventing it from being modified further.

It may work to have something like this in main():

int main(int argc, char** argv)
{
    if (argc != 2)
    {
        // let the user know of the correct input
        fprintf(stderr, "usage: %s string\n", argv[0]);
        return -1;
    }

    const char* original = argv[1];
    const char* compressed = compress(original);

    printf("The compression of %s is %s \n", original, compressed);

    free(compressed);
}

Code Snippets

if (argc != 2)
{
    fprintf(stderr, "Not correct number of inputs.");
    return -1;
}
for (; str[i] == str[i+1]; i++)
{
    //to store number in char array, add the asci value 
    // of 0.
    result[j] = (++counter + '0');
}
char* compress(char* str)
{
    // check to make sure str is valid...

    // assuming len is also valid
    char* result = malloc(len);

    // OR, use calloc() instead to get the memory zeroed
    //   thus no need for memset() or anything else
    char* result = calloc(len, sizeof(char));

    // probably also good to check for sufficient memory
    // if insufficient, print error and abort
    if (!result)
    {
        perror("malloc"); // or "calloc"
        abort();
    }

    // do the compression...

    // return it right away; no need to copy anything
    return result;
}
int main(int argc, char** argv)
{
    if (argc != 2)
    {
        // let the user know of the correct input
        fprintf(stderr, "usage: %s string\n", argv[0]);
        return -1;
    }

    const char* original = argv[1];
    const char* compressed = compress(original);

    printf("The compression of %s is %s \n", original, compressed);

    free(compressed);
}

Context

StackExchange Code Review Q#115538, answer score: 8

Revisions (0)

No revisions yet.