patterncMinor
Extracting the longest line from standard input
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:
My code:
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,
Think carefully about variable use
After the first line is read, the variable
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
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
Choose better variable names
The name
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:
However, that's more idiomatically written like this:
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:
use a bit more whitespace and write it like this:
Doing so will make it easier to read and understand your program.
Eliminate
Since C99, the compiler automatically generates the code corresponding to
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 mainSince 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.