Extracting the longest line from standard input

Question

I'm reading the K&R book and I'm interested in a program which finds the longest line on the input. I've written the same code as in the book, but I wanted to do it myself. I tried to write my own code to do same thing. I think it's working, but my question is whether my code is "good" or if it's written in a "bad way" and can be written easily.

K&R Code:

#include <stdio.h>
#define MAXLINE 1000
int getline(char line[], int maxline);
void copy(char to[], char from[]);
main () {
 int len;
 int max;
 char line[MAXLINE];
 char longest[MAXLINE];
 max = 0;
 while ((len = getline(line, MAXLINE)) > 0) {
 if(len > max) {
 max = len;
 copy(longest, line);
 }
 }
 if(max > 0) printf("%s", longest);
 return 0;
}
int getline (char line[], int limit) {
 int i, c;
 for (i = 0; i < limit - 1 && (c = getchar()) != EOF && c != '\n'; i++) line[i] = c;
 if (c == '\n') {
 line[i] = c;
 i++;
 }
 line[i] = '0円';
 return i;
}
void copy(char to[], char from[]) {
 int i;
 i = 0;
 while((to[i] = from[i]) != '0円')
 i++;
}

My code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define MAX_LINE 1000
int main()
{
 int ch; //char input
 int length; //length of the line
 int max=0; //length of the longest line
 int j=0;
 int i=0;
 char line[MAX_LINE]; //line input
 char help [MAX_LINE]; 
 char longest [MAX_LINE]; //longest line
 while ((ch=getchar())!='\n'){ //first line
 longest[i]=ch;
 ++i;
 }
 max=i;
 i=0;
 length=0;
 while ((ch=getchar())!=EOF){
 line[i]=ch;
 help[j]=ch;
 ++j;
 ++i;
 ++length;
 if (ch=='\n'){
 help[j]='0円';
 if (length-1>max){
 max=length-1;
 j=0;
 printf("Longest -> %s",help);
 }
 else {
 memset(help,0,100); //erase elements
 }
 length=0;
 }
 }
 return 0;
}

• CiaPan: What happens to your code if the input consists of a single line not terminated with a new-line...? IMVHO it will stuck in the while ((ch=getchar())!='\n') loop.
• Dan: Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers .
• Vogel612: I have rolled back the latest chages with the same reasoning as the rollback Dan performed. Please revisit the link that Dan provided. Thanks :)

Answer 1

I see some things that may help you improve your code.

Eliminate unused variables

Unused variables are a sign of poor quality code, and you don't want to write poor quality code. In this code, line and longest are unused -- their values are set but then never referenced again. Your compiler is smart enough to tell you about this if you ask it nicely.

Think carefully about variable use

After the first line is read, the variable i isn't really needed. The way it is now, it could instead simply increment max directly.

Think carefully about the task

If you need to save the longest line, you can't overwrite it every time. The difference between yours and the K&R version is that yours prints multiple times and only the last time it prints is actually the longest line.

Check for unusual inputs

What happens if the file consists of a single long line? The existing code will fail because the code that reads the first line fails to stop at the end of the file. Also none of the code checks for lines that are too long.

Break up the code into smaller functions

The main() function does a series of identifiable steps. Rather than having everything in one long function, it would be easier to read and maintain if each discrete step were its own function.

Don't repeat yourself

There's really no need to handle the first line any differently than any other line. Instead, you could simply run the main loop.

Minimize work

There's no reason to clear the entire line array. Instead of the memset, your code could simply do this:

help[0] = '0円';

Choose better variable names

The name help is not at all appropriate to its actual function. I'd recommend using line for everywhere that help is currently used.

Use modern syntax

The old K&R style was to declare all variables at the top of the function, but that's not any longer either required or desired. Instead, current practice is to declare variables near their first use and to keep their scope small.

Use modern C idioms

Right now the code contains these lines:

help[j]=ch;
++j;

However, that's more idiomatically written like this:

help[j++] = ch;

Fix your formatting

There are inconsistent spaces in variable declarations, and inconsistent indentation. Being consistent helps others read and understand your code.

Use more whitespace

Instead of writing a line like this:

while ((ch=getchar())!=EOF){

use a bit more whitespace and write it like this:

while ((ch = getchar()) != EOF) {

Doing so will make it easier to read and understand your program.

Eliminate return 0 at the end of main

Since C99, the compiler automatically generates the code corresponding to return 0 at the end of main so there is no need to explicitly write it.

• phos456: There was a problem with` help[j++] = ch;` . When I finished the input ,it always printed strange character before the longest line. What do you think about my new code? I tried to fix it with some of your improvements.
• alx - recommends codidact: "Eliminate return 0 at the end of main": I would say no. Saving just one line of code in all your program is not enough reason, and maybe I have a bit of an OCD, but I like to see it; it feels consistent :).
• Edward: As a style point, it's an endless debate. However either way one views it, it's still useful to know that the standard specifically says that omitting it is exactly equivalent to including it. I also omit writing return; at the end of void functions for exactly the same reason -- reducing visually distracting clutter that has zero semantic content.

Answer 2

Structure

• I would not separate reading the first line from the rest. You could just initialize max to zero.

• Functions: I prefer the K&R solution, to have smaller functions. This makes the program more easy to read and understand.

Output

• The program will write each line that happens to be longer than the actual max. length. Not just the longest one.

Various

• This won't work correctly if helper is longer than 100: memset(help,0,100);. I recommend using the constant instead. (Btw., it is really good that you do have the constant already.)

• help variable: I think lineWithTerminator would be slightly better. Also, I do not really understand, why this variable is needed at all? Why not just reuse line? If there is a good reason for that, I suggest documenting it in a comment (i.e documenting the reason, why that extra variable is needed).

Possible improvement

You could even further improve either your solution, or that of the book to use dynamic allocation (malloc/realloc/free) instead of fixed size arrays.

Answer 3

main

C17::6.11.6:

###Function declarators

1. The use of function declarators with empty parentheses (not prototype-format parameter type declarators) is an obsolescent feature.

(this doesn't really affect definitions, but better be consistent between declarations and definitions).

And the new standards removed the implicit int rule (I don't know if it was C11 or C99).

Use int main(void).

bug

CiaPan found a bug. Here are three proposals to fix it.

1.  for (c = getchar(); (c != '\n') && (c != EOF); i++) {
    longest[i] = c;
    c = getchar();
    }
   

2.  c = getchar();
    while ((c != '\n') && (c != EOF)) {
    longest[i++] = c;
    c = getchar();
    }
   

3.  while (((c = getchar()) != '\n') && (c != EOF))
    longest[i++] = c;
   

(削除)

macros parenthesized

Although sometimes it's not needed, for safety reasons it's better to parenthesize macros always (at least macros that evaluate to a value):

#define MAX_LINE (1000)

(削除ここまで)

see @chux's comment.

ptrdiff_t or size_t

Variables than mean array indices should preferably be of one of these types.

loop local variables

Since C99 you can create variables local to a loop. That helps removing some lines of code.

for (ptrdiff_t i = 0; (c = getchar()) != EOF; i++) {
 ...
}

magic numbers

Don't write any number different than 0, 1, or 2 in any part of your code apart from constant macros: #define FOO (9) is the only place where numbers deserve to go. Sometimes there are exceptions to that rule, but you will know the time when it comes.

If you see any magic number in your code, think of a good name for it and write a macro.

• chux: "for safety reasons it's better to parenthesize macros always (at least macros that evaluate to a value" --> Adding () to the constant 1000 prevent stringification to "1000" that is useful with scanf() format and others.
• alx - recommends codidact: @chux Hmm, that's right. Please feel free to strikeout the whole paragraph in my answer, as I don't know how to do it.
• chux: Use <strike>text</strike>. Outside () are good, as a general rule. I find numeric constants an exception.