4
\$\begingroup\$

I am working on a program that is meant to fold a large input line into smaller ones. Basic flow of the program would be as follows:

  • User calls the program with a long string as input
  • Program prints tokens generated in separate lines

I haven't seen handling file input yet, so keep that in mind. I haven't seen reading input from the command line either.

Here's the source code:

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

int length(char input[]);

int main(int argc, char* argv[])
{

    if(argc < 3)
    {
        printf("Usage: %s input [n]\n", argv[0]);
        exit(1);
    }

    int COL_BOUND = atof(argv[2]);

    int startCol, currentCut, nextBound, numOfCuts;

    startCol = currentCut = nextBound = numOfCuts = 0;

    //while the next bound is still "inside" string
    while((nextBound = startCol + COL_BOUND - 1) <= length(argv[1]))
    {
        //find cut character
        for(int i = startCol; i <= nextBound; ++i)
        {
            if(isspace(argv[1][i]) && !isspace(argv[1][i-1]))
                currentCut = i - 1;
        }
        //there was no valid "cut" before whitespace or bound itself is valid
        if(!isspace(argv[1][nextBound]) || currentCut <= startCol)
            currentCut = nextBound;

        //print [startCol, cut]
        int printed = 0;
        for(int i = startCol; i <= currentCut; ++i)
        {
            if(isspace(argv[1][i]) && printed == 0)
                continue;
            ++printed;
            putchar(argv[1][i]);
        }

        printf("\n");

        //adjust start column for the next run
        startCol = startCol + COL_BOUND;
        ++numOfCuts;
    }

    //print remaining characters
    int printed = 0;
    ++numOfCuts;
    for(int i=startCol; i <= length(argv[1]); ++i)
    {
        if(isspace(argv[1][i]) && printed == 0)
            continue;
        ++printed;
        putchar(argv[1][i]);
    }

    printf("\nOriginal string split into %d parts (bound was %d)\n", numOfCuts, COL_BOUND);
    return 0;
}

int length(char input[])
{
    int i;
    for(i=0; input[i] != '\0'; ++i)
        ;
    return i;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Why does code covert the string int COL_BOUND = atof(argv[2]); to a float and then assigned to an int? \$\endgroup\$ – chux May 11 '18 at 2:41
  • 2
    \$\begingroup\$ I am not proficient on reading input from the console, as I've noted above. The reason its assigned to an int is because it is used as an integer value - the column bound. A column bound would never have a decimal part to it; it's either on one character or the other. Edit: Upon further inspection, man atof revealed that its integer equivalent is indeed atoi. Thank you for pointing this out. \$\endgroup\$ – M. Lago May 11 '18 at 2:47
3
\$\begingroup\$

The length() function can be replaced using strlen() (you'll need to include <string.h>). Note that strlen() doesn't consider the terminating NUL to be part of the string - but that will help fix the current bug that causes this code to print that NUL.

Don't measure the length repeatedly - instead, save it to a variable early on, and refer to that variable.


Error messages should go to standard error stream, not standard output:

if (argc < 3) {
    fprintf(stderr, "Usage: %s input cols\n", argv[0]);
    return 1;
}

Don't use uppercase names for variables - we reserve those for macros; the "SHOUTING" of the name warns us that they need special care.

Also, don't use atof() for converting integers - use atoi(), or better, strtoul(), which can tell you whether it succeeded:

char *parse_end;
size_t col_bound = strtoul(argv[2], &parse_end, 10);
if (parse_end == argv[2] || *parse_end) {
    fprintf(stderr, "Usage: %s input cols\n", argv[0]);
    return 1;
}

Prefer one declaration per line, and initialise immediately:

size_t startCol = 0;
size_t currentCut = 0;
size_t nextBound = 0;
size_t numOfCuts = 0;

Be careful about off-by-one errors. This inequality should be <, not <=:

while ((nextBound = startCol + col_bound - 1) < length) {

It's not the only one.


Test more inputs. I tried a very simple case and got unexpected output:

./194156 'a b c d e f' 3
a b
c
d e
f
Original string split into 4 parts(bound was 3)

Why did it not split into 3 lines?

\$\endgroup\$
3
\$\begingroup\$

Command-line tool behavior

Don't contaminate stdout, which should be solely for the string-processing result. Error messages and status reports should go to stderr.

In the usage message, what does input mean? Is it a filename? I think that string would be clearer. Similarly, maxwidth would be clearer than [n]. Note that by convention, the square brackets mean that it's an optional parameter, which it isn't.

Calling exit(1) from main() is overkill — return 1 would suffice.

You can parse argv[2] as an integer using atoi(). COL_BOUND is not a compile-time constant, so it should be named with lowercase. Note that if COL_BOUND is 0, then you'll get an infinite loop, so some validation would be nice.

Whitespace behavior

What exactly is the specification of this program? Here are three sample runs (with space () and NUL () characters made visible):

$ ./fold "␣hello␣␣␣␣world" 5
hell
o␣␣␣␣
world
␀
Original string split into 4 parts (bound was 5)
$ ./fold "␣hello␣␣␣␣␣␣world" 5
hell
o␣␣␣␣
wor
ld␀
Original string split into 4 parts (bound was 5)
$ ./fold "␣␣hi␣mom" 2

hi
m
om
␀
Original string split into 5 parts (bound was 2)

The behavior is weird in certain ways:

  • I see that the policy is to print each line with no leading whitespace. But then why is the first line "hell" rather than "hello"? Why does it print "wor" instead of "world"? Why does it print "m","om" rather than "mo","m"?
  • If excess whitespace is considered insignificant, then in the third run, why is there an empty line at the beginning?
  • Sometimes there is trailing whitespace, sometimes there isn't.
  • You actually print the NUL character at the end. (This bug is due to your i <= length(argv[1]) test, which should be < instead.) Sometimes that NUL character becomes its own extra line of output.

Implementation

By convention, main() should be defined after the helper functions, so that you don't have to write forward declarations.

Your length() function is just a reimplementation of the standard strlen() function. You call it every time you go through the main while loop, which is very inefficient. Ideally, you should perform this task without measuring the length at all — it's possible to analyze the string in one single forward pass, stopping when you reach the NUL terminator.

Other than the useless length() function, all of the code is in main(), which makes it one very complex function. One of the causes of your buggy behavior is that you try to do whitespace analysis (with printed) while printing, when it's clearly too late.

You have a //print remaining characters epilogue loop, which is not only ugly and repetitive, but also makes it hard to handle degenerate cases correctly (such as when the input is an empty string).

Suggested solution

I'd define a nextline() function to find the start and end of the next line. Its design is vaguely inspired by the strsep(3) function in BSD's C library. Then, main() would just be responsible for handling the command-line parameters and printing the output.

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

/**
 * Finds the start and end of the next line, to be terminated at whitespace,
 * up to a certain length.
 *
 * First, s is adjusted to skip any leading whitespace.  Then, a suitable
 * line-breaking point is found and returned, such that the line is at most
 * maxlen characters long, and trailing whitespace is dropped.
 */
char *nextline(char **const s, size_t maxlen) {
    // Skip leading whitespace
    while (**s && isspace(**s)) {
        (*s)++;
    }

    // Find last whitespace within the remaining string length and within
    // maxlen characters, but ignoring extraneous consecutive whitespace
    char *lastspace = *s + maxlen;
    for (char *p = *s; p <= *s + maxlen; p++) {
        if (*p == '\0') {
            return p;
        }
        if (isspace(*p) && !isspace(*(p - 1))) {
            lastspace = p;
        }
    }
    return lastspace;
}

int main(int argc, char *argv[]) {
    if (argc < 3) {
        fprintf(stderr, "Usage: %s string maxwidth\n", argv[0]);
        return 1;
    }

    char *start = argv[1];
    int width = atoi(argv[2]);
    if (width <= 0) {
        fprintf(stderr, "maxwidth must be positive\n");
        return 1;
    }

    int linecount = 0;
    for (char *end; end = nextline(&start, width), *start; start = end) {
        // Temporarily NUL-terminate the line to print it
        char termchar = *end;
        *end = '\0';
        puts(start);
        *end = termchar;
        linecount++;
    }

    fprintf(stderr, "Split string into %d parts (bound was %d)\n",
            linecount, width);
}
\$\endgroup\$

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.