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\$
  • 1
    \$\begingroup\$ I don't believe that this code does what the question's title suggests. Have you tested it? \$\endgroup\$ – 200_success Mar 17 '16 at 5:51
  • \$\begingroup\$ @200_success: It should say at least 80 characters. (And it only works for a single line.) \$\endgroup\$ – Thomas Padron-McCarthy Mar 17 '16 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\$ – Thomas Padron-McCarthy Mar 17 '16 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\$ – Lúcio Cardoso Mar 17 '16 at 9:53
  • \$\begingroup\$ It has a BUG: printf("%s", line); but line is not null-terminated. \$\endgroup\$ – Paul Ogilvie Mar 22 '16 at 14:14
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);
        }
    }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ @JerryCoffin "4095 characters in a string literal (after concatenation)" doesn't apply here? \$\endgroup\$ – syb0rg Mar 17 '16 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 '16 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\$ – Jerry Coffin Mar 17 '16 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\$ – Thomas Padron-McCarthy Mar 17 '16 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\$ – clem steredenn Mar 18 '16 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).

| improve this answer | |
\$\endgroup\$
  • 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\$ – Thomas Padron-McCarthy Mar 17 '16 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\$ – chux - Reinstate Monica Mar 20 '16 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\$ – chux - Reinstate Monica Mar 20 '16 at 23:39

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.