4
\$\begingroup\$

I have an exercise in my ANSI C book:

3.3. Write a function expand(s1,s2) that expands shorthand notations like a-z in the string s1 into the equivalent complete list abc…xyz in s2. Allow for letters of either case and digits, and be prepared to handle cases like a-b-c and a-z0-9 and -a-z. Arrange that a leading or trailing - is taken literally.

I have attempted to solve it and here's my code:

#include <stdio.h>
#include <string.h>
#define MAXLEN 100

void expand(char * s1, char * s2);

int main(void) {
    char s1[] = "a-b-c";
    char s2[MAXLEN];
    expand(s1, s2);
    printf("%s\n", s2);
    return 0;
}

void expand(char * s1, char * s2) {
    char prev = '\0';
    char mid = '\0';
    char next = '\0';
    if (s1[0] == '-') {
        *s2 = *s1;
        s1++, s2++;
    }
    while (*s1 != '\0') {
        if (next == '\0' && mid == '-')
            *s2++ = mid;
        else if (mid == '-') {
            if (prev < next)
                for (char i = prev; i <= next; i++, s2++)
                    *s2 = i;
            else
                for (char i = prev; i >= next; i--, s2++)
                    *s2 = i;
        }
        prev = *s1;
        s1++;
        mid = *s1;
        if (*s1 != '\0')
            next = *(s1 + 1);
    }
    *s2 = *s1;
}

What do you think about this solution? Does it solve the exercise? Is it a good way to solve it?

\$\endgroup\$
  • \$\begingroup\$ Well, you are writing in ANSI C, not K&R C. Your title reads “K&R C book”? \$\endgroup\$ – JDługosz Jun 9 '18 at 22:28
  • \$\begingroup\$ They are probably reffering to the second edition. K&R C is named after the C shown in "The C Programming Language". The second edition is modified to use ANSI C. \$\endgroup\$ – Peter Jun 23 '18 at 21:45
  • \$\begingroup\$ This is a terrible exercise. I don't understand the instructions at all. Does "be prepared to handle cases like a-b-c" mean the result would be abbc? or abc? And what does "Arrange that a leading or trailing -is taken literally." mean? Is it intended that when part of the string is not a range, that you simply copy it? \$\endgroup\$ – user1118321 Jul 10 '18 at 4:42
1
\$\begingroup\$
#include <string.h>

We don't use any of the definitions of this header, so it can safely be omitted.


#define MAXLEN 100

Why 100? More importantly, what happens when we reach this maximum length? We probably need to add some code to stop producing output when it's reached. The provided interface expand(s1,s2) doesn't allow us to tell the function how much space is available to write into s2, so that's probably out of scope for the exercise, but consider implementing expand(s1,s2,max_len) as a further exercise (and a good one, that will be useful in your future C coding).


What should the output be for a-b-c? Your program produces abbc, but I don't know if that's expected. At least add a comment to document your assumptions. Other possible interpretations include abc and ab-c, so explain why you've chosen abbc for this input.


Another problematic test case: --5.


Use const for the input string. If we also return the start of the output, we can use the function more naturally:

char *expand(const char *s1, char *s2) {
    char *const s2_start = s2;

    /*...*/

    return s2_start;
}

int main(void) {
    char s2[MAXLEN];
    puts(expand("--5", s2));
}

Note that I've used puts() as a simpler alternative to sprintf("%s\n", ).


Your indexing is odd and inconsistent. I see s1[0] mixed in with *s, and *(s1 + 1) in place of the simpler s1[1]. You can also make use of the value of increment expressions, like this:

if (*s1 == '-') {
    *s2++ = *s1++;
}
            char i = prev;
            if (prev < next)
                while (i <= next)
                    *s2++ = i++;
            else
                while (i >= next)
                    *s2++ = i--;

Improved code

I've completely re-written, observing the above changes:

#include <stdio.h>

char *expand(const char *s1, char *s2, int max_len) {
    char *const s2_start = s2;
    char *const s2_end = s2_start + max_len - 1;

    while (*s1 && s2 < s2_end) {
        if (*s1 == '-' && s2 != s2_start && s1[1]) {
            /* spotted hyphen not at beginning or end */
            char first = s1[-1];
            char last = *++s1;
            /* produce a range first..last,
               exclusive (we've already written first) */
            if (first == last) {
                ++s1;
            } else if (first < last) {
                /* ascending */
                for (char i = ++first;  i < last && s2 < s2_end;  ++i)
                    *s2++ = i;
            } else {
                /* descending */
                for (char i = --first;  i > last && s2 < s2_end;  --i)
                    *s2++ = i;
            }
        } else {
            /* copy to output */
            *s2++ = *s1++;
        }
    }
    *s2 = '\0';

    return s2_start;
}


#define MAXLEN 100
int main(void) {
    char s2[MAXLEN];
    puts(expand("a-z-a-z-a", s2, MAXLEN));
}
\$\endgroup\$
0
\$\begingroup\$
void expand(char * s1, char * s2);

Although the placement of * (int * a or int* a or int *a), I would stick to one and only one throughout a codebase. Later on you have *s2 = *s1 so it's a good idea to use that style in the function signature as well.

\$\endgroup\$
  • 1
    \$\begingroup\$ But those are different cases. int * a is a parameter declaration, whereas *s1 = *s2; is an assignment. There's no reason you should expect them to be the same, in my opinion. \$\endgroup\$ – user1118321 Jul 10 '18 at 4:39
  • \$\begingroup\$ @user1118321 But they are not 'different cases'. The C syntax of type declaration is explained as mimicking the expression context: int *x declares the x identifier to provide int value if used in the *x expression. OTOH surrounding the asterisk with spaces suggests the multiplication operator, which is misleading here. \$\endgroup\$ – CiaPan Jul 10 '18 at 10:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy