1
\$\begingroup\$

How can I improve this program?

#include <stdio.h>
#define MIN 80

int main(void)
{
    char line[MIN];
    int i, c;

    for (i = 0; (c = getchar()) != EOF && c != '\n'; ++i)
    {
        if (i < MIN - 1)
            line[i] = c;
        else
        {
            if (i == MIN - 1)
                printf("%s", line);

            printf("%c", c);
        }

    }

    return 0;
}
\$\endgroup\$
5
  • 1
    \$\begingroup\$ I don't believe that this code does what the question's title suggests. Have you tested it? \$\endgroup\$ Mar 17, 2016 at 5:51
  • \$\begingroup\$ @200_success: It should say at least 80 characters. (And it only works for a single line.) \$\endgroup\$ Mar 17, 2016 at 6:34
  • \$\begingroup\$ @LúcioCardoso: Something that would improve the program greatly is a clear statement of what it is supposed to do. I think Jamal, who edited the title, got it almost right, but not quite. So it is not obvious - and if there are bugs in the code, it may be impossible to guess what the intention was. \$\endgroup\$ Mar 17, 2016 at 6:38
  • \$\begingroup\$ The program should print a line if it's greater than 80 characters. I should only write the line, the program will then test if it's characters quantity is at least 81. Then, if it's true, the program will print the line. \$\endgroup\$ Mar 17, 2016 at 9:53
  • \$\begingroup\$ It has a BUG: printf("%s", line); but line is not null-terminated. \$\endgroup\$ Mar 22, 2016 at 14:14

2 Answers 2

6
\$\begingroup\$

A few notes (overall very good for a beginner):

  • Change MIN so that it's a static const because it respects scope and is type-safe. Also, what is it the minimum for? The variable name could be more specific.

  • Very nice on the int main(void)!

  • Always initialize your variables when you declare them. This helps prevent unintended behavior later on in the program.

  • Declare i within your for loop, this should be standard by now (and has been as of C99).

    for (int i = 0; ...)
    

    You could also do this with your c variable, but I wouldn't recommend that as i is the counter that is tied directly to that for loop.

  • The rest of your for loop may be hard for some beginners to read, but I actually like it the way it is and don't find it too hard to read.

  • The way you have your if statement may be of concern to some.

    if (i < MIN - 1)
        line[i] = c;
    

    Their argument is that it could lead to potential bugs in the future if maintained incorrectly. I'm going to say it's fine, as long as you're consistent with it AND use GCC 6's -Wmisleading-indentation flag to help catch those problems with it in the future

  • You don't append \0 to line when you are about to print it.

    if (i == MIN - 1)
    {
        line[i] = '\0';
        printf("%s", line);
    }
    
  • You don't have to return 0 at the end of main(), just like you wouldn't bother putting return; at the end of a void-returning function. The C standard knows how frequently this is used, and lets you not bother.

    C99 & C11 §5.1.2.2(3)

    ...reaching the } that terminates the main() function returns a value of 0.


Final code:

#include <stdio.h>

static const int MIN_LENGTH = 80;

int main(void)
{
    char line[MIN_LENGTH];
    int c = 0;

    for (int i = 0; (c = getchar()) != EOF && c != '\n'; ++i)
    {
        if (i < MIN_LENGTH - 1) line[i] = c;
        else
        {
            if (i == MIN_LENGTH- 1)
            {
                line[i] = '\0';
                printf("%s", line);
            }

            printf("%c", c);
        }
    }
}
\$\endgroup\$
9
  • \$\begingroup\$ @JerryCoffin "4095 characters in a string literal (after concatenation)" doesn't apply here? \$\endgroup\$
    – syb0rg
    Mar 17, 2016 at 1:15
  • \$\begingroup\$ @JerryCoffin The "after concatenation" I guess was throwing me off. I questioned it as first but I thought it still applied. Answer has been edited. \$\endgroup\$
    – syb0rg
    Mar 17, 2016 at 1:20
  • 1
    \$\begingroup\$ The "after concatenation" is referring to the fact that adjacent string literals get concatenated, so source code like char const *c = "one" " two" " three"; produces a single string literal containing "one two three". This is typically used for things like long (multiple lines) strings, and a few cases like the macros for printf format strings for things like size_t. \$\endgroup\$ Mar 17, 2016 at 1:23
  • \$\begingroup\$ @syb0rg: Always compile and test exactly the code that you publish, by copying and pasting. Otherwise there will always be errors in it. In this case, a missing =, many MIN instead of MIN_LENGTH, and C doesn't allow you to initialize the variable-sized array line. \$\endgroup\$ Mar 17, 2016 at 6:12
  • \$\begingroup\$ Older compilers won't like the declaration of i within the for loop. And it goes against older standards (ANSI C), if I'm not mistaken. \$\endgroup\$ Mar 18, 2016 at 6:45
2
\$\begingroup\$

If I were doing this, I'd do it somewhat differently. The usual way to read a line of input is to use fgets. fgets is fairly carefully written to support reading lines that are longer than the buffer you provided (and that support will work fine here).

To support long lines, fgets leaves the entire line intact--including the new-line character that signals the end of the line. So, if it reads 81 characters, and no newline has been encountered yet, we know the line is over 80 characters long. Based on that, we can write code that seems (at least to me) to express our intent a little more closely:

static const int max = 82;
char buffer[max];

while (fgets(buffer, max, stdin)) {
    if (strlen(buffer) > 80 && buffer[81] != '\n') {
        fputs(buffer, stdout);
        int ch;
        while ('\n' != (ch=getchar()) && EOF != ch)
            putchar(ch);
}

As it stands, this is marginally less efficient, since it re-scans through each line of input to find the string length. This is typically irrelevant (scanning is typically much faster than I/O), but if we really care about it, we can eliminate that as well.

To do that, we set the second to last character in the buffer to a new-line before calling fgets. Then we look at (only) that character after the fgets. If we read a line < 80 characters long, it will still contain the new-line we put there. If we read a line exactly 80 characters long, our new-line will be overwritten with an NUL terminator ('\0'). If the input line exceeds 80 characters, it will be overwritten with some other value (so then we print it out, and copy the remainder of the line as well).

\$\endgroup\$
3
  • 1
    \$\begingroup\$ char ch; should be int ch; to guarantee that the EOF test works correctly. Also, this program does not work the same as the original one. It handles more than one line, and a line with 80 characters and no newline is not printed. But still +1 for suggesting to use existing library functions. \$\endgroup\$ Mar 17, 2016 at 6:43
  • \$\begingroup\$ Pedantically, this does not work as the detection for the number of characters read uses strlen(buffer) which does not detect if a null character was read. IAC, fgets() reading null characters is a problem anyways. \$\endgroup\$ Mar 20, 2016 at 23:35
  • \$\begingroup\$ "set the second to last character in the buffer" is a reasonable, yet counts on behavior not defined by C. C does not specify the remainder of the buffer is left unmodified. I have not come across a system the does alter the "later" bytes, yet I see nothing that prevents a system from doing so. IMO - a weakness in fgets() definition. \$\endgroup\$ Mar 20, 2016 at 23:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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