String-crunching routine How can this be optimized?

char *crunch(char *s)
{
  char *temp,buff[MAX];
  int repeat,count,i,j,k;

  if (*s == '\0')
    return NULL;
  temp = (char*)malloc((strlen(s) + 1) * sizeof(char));
  if (isdigit(*s))
    return NULL;
  repeat = 0;
  for (i = 0, j = 0;i &lt; strlen(s);)
  {
    temp[j++] = s[i++];
    count = 1;
    while (s[i] == s[i - 1])
    {
      count++;
      i++;
      repeat = 1;
    }
    if (isdigit(s[i]) &amp;&amp; repeat == 1)
      return NULL;
    if (isdigit(s[i]) &amp;&amp; count != 1)
      return NULL;
    else if (isalpha(s[i]) &amp;&amp; count != 1)
    {
      temp[j++] = count + 48;
    }
    else if (isdigit(s[i]))
    {
      for (k = 1;k &lt; (s[i] - 48);k++)
        temp[j++] = s[i - 1];
      i++;
    }
  }
  temp[j] = '\0';
  return temp;
}


Examples:

Input: "aabbccdef"
Output: "a2b2c2def"

Input: "a4bd2g4"
Output: "aaaabddgggg"
As written by Jonathan and


Always put a blank line after every }
Always use { } even for single line if/for/while. There is a class of errors where you add a second line in the if but it isn't "in" the if, it is outside. Something like:




if (1 == 1)
   do something


and then you change it in 

if (1 == 1)
    do something
    do something else


the do something else isn't in the if, even if you spaced it like it was :-)

If you don't want to use the {} a good style that solves it is to put the second part of the instruction in the same line

if (1 == 1) do something


I don't like it very much (because it makes the line longer to the right) but it is more difficult you make the "bad" error of missing {}

The strlen can be pre-calculated once. 

The if (isdigit(*s)) can be put before the malloc.

If you malloc a block of memory and then return NULL you should free the block of memory, otherwise you cause a memory leak.

Please, don't use the pre-fix/post-fix increment operator in the middle of other instructions, they make the code less readable. 

This

temp[j++] = s[i++];


is the same as

temp[j] = s[i];
i++;
j++;


but the second is much more readable, because you don't have to think to the order of the operations.

If you have to decompress the string (from a4bd2g4 to aaaabddgggg) the buffer size is too much small.

Considering the compression and decompression are quite different, I would do it in two passes:

first pass: sum all the digits and count all the characters. If the sum is zero the it's compression, do it in function 1. If sum &lt;> 0 it is decompression. With the sum of the digits and the count of the characters you know the size of the buffer you'll need.

If you want to impress your teacher, you should make it work for aaaaaaaaaa (it is 10x a)... It would compress to a9a and aaaaaaaaaaa (11x a) a9a2.