Shorthand expansion exercise from K & R C (ex 3-3)

I have written a function to take some shorthand input and expand it into the full form, for example a-z0-9 becomes abcd...789. I added protection from buffer overflow exploits (knocks on wood), and have covered (or think I have) all and any edge cases that the user could throw at it.

Are there any ways in which my code could be improved (subjective and objective criticism welcome here)? Have I missed an edge case? are there any insecure sections of my code that might allow for a buffer overflow? Is my code written in a fairly concise and readable way?

// expand.c - function expand(s1, s2) takes shorthand input (i.e. a-z0-9) and expands it, placing the result in s2 (i.e. abcde...789)
#include 
#define MAXLEN 256
#define UP 1    //Included only to help improve readability 
#define DOWN -1 //Ditto

void expand(char s1[], char s2[]);

int main()
{
    char output[MAXLEN], input[MAXLEN];
    printf("Enter string to expand: ");
    fgets(input, MAXLEN + 1, stdin);
    expand(input, output);
    printf("%s\n", output);
    return 0;
}

void expand(char s1[], char s2[])
{
    int i, j, k, count;
    for(i = j = 0; s1[i] != '\0' && i < MAXLEN && j < MAXLEN; i++) {
        if(s1[i + 1] == '-' && s1[i + 2] != '\0') {
            if(s1[i] < s1[i + 2])
                count = UP;
            else
                count = DOWN;
            for(k = 0; s1[i] + k - count != s1[i + 2] && j < MAXLEN; k += count, j++) {
                if(s1[i - 1] == '-' && k == 0)
                    j--;
                s2[j] = s1[i] + k;
            }
            s2[j] = '\0';
        }
    }
}

Here are some comments regarding your code:

• Using s1[i+2] and s1[i-1] can possibly refer to memory locations outside of allocated memory

• Inside of expand() you use the restriction of MAXLEN which is not conveyed in the parameters, which should be like char s1[MAXLEN] (which also introduces different strange aspect of the c-language). As your code stands now you can easily call it with char fails[2]; expand(fails, fails)

• You should also test for null before accessing input parameters, currently you can call expand(input, 0) and it will happily try to do something which will fail

• You are better of to always include braces around blocks within if-else statements to avoid erroneous indentation errors

• Give s1 and s2 proper names to indicate what they are

• You'll possibly get into problems or unwanted behaviour, with input like z-0, a-00--a, Z-0, --[

• The range a-a will give count = DOWN, and behave somewhat strangely

Regarding the brace comment, consider the following code snippet:

int a = 13; int b = 5;
if (a  b)
      a = 5;
else
   b = 6;

What will a and b hold now? Correct answer is a=5, b=5. If surrounding the blocks with braces the code would behave like you possibly read it, indicating the correct response of a=13, b=6. This is a constructed example, but it does happen way to often in real life that someone indents the code wrongly and strange things happen.

Here is the code example again, using starting braces on the same line, many prefer them to be on a new line. Choose a style, and stick to it.

int a = 13; int b = 5;
if (a  b) {
      a = 5;
   }
} else {
   b = 6;
}

StackExchange Code Review Q#100418, answer score: 5