HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Extracting the longest line from standard input

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
fromthelinestandardinputextractinglongest

Problem

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 
#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 
#include 
#include 
#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;
}

Solution

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.

Code Snippets

help[0] = '\0';
help[j]=ch;
++j;
help[j++] = ch;
while ((ch=getchar())!=EOF){
while ((ch = getchar()) != EOF) {

Context

StackExchange Code Review Q#129702, answer score: 7

Revisions (0)

No revisions yet.