6
\$\begingroup\$

Like many posts before, I'm going through The C Programming Language, 2nd Edition, by Kernighan and Ritchie.

I am very new to the c programming language. While I have a somewhat decent understanding of programming basics, I'm most looking forward to learning about memory management in c.

Anyway, I'm looking for feedback on my latest exercise solution from the book.

Exercise 1-17. Write a program to print all input lines that are longer than 80 characters.

My initial solution:

#include <stdio.h>
#include <string.h>

#define MINCHARS 80
#define MAXLINE 1000

int getLine(char line[], int size);

int main() {
    char line[MAXLINE];
    char minCharsLine[MAXLINE][MAXLINE];
    int lineCharCount[MAXLINE];
    int c;
    int len;
    int lineCount;
    len = 0;
    lineCount = 0;

    while ((c = getchar()) != EOF) {
        line[len] = c;
        len++;
        if (len > MINCHARS) {
            if (c == '\n') {
                lineCount++;
                strcpy(minCharsLine[lineCount], line);
                lineCharCount[lineCount] = len;
                len = 0;
            }
        } else if (c == '\n') {
            len = 0;
        }
    }
    if (lineCount > 0 ) {
        for (int i = 0; i < lineCount + 1; i++) {
            for (int j = 0; j < lineCharCount[i]; j++) {
                printf("%c", minCharsLine[i][j]);
            }
        }
    } else {
        printf("No lines over 80 characters\n");
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ "all input lines that are longer than 80 characters" --> does that 80 include the potential '\n'? \$\endgroup\$ Jan 5 at 4:53
  • \$\begingroup\$ "I'm most looking forward to learning about memory management in c." Ah, a fellow programming-masochist I see. \$\endgroup\$
    – pipe
    Jan 5 at 14:28

1 Answer 1

5
\$\begingroup\$

Nice layout/formatting

Object names are reasonable.

Saving more than about 80 characters is not needed

Once the "80" line length is reach, print the first 80 characters and then the rest of incoming characters until end-of-line/end-of-file.

char minCharsLine[MAXLINE][MAXLINE], int lineCharCount[MAXLINE] not needed.

Good that code saved the return value of getchar() in an int

getchar() returns 257 different values*1. Saving in a char can lose information.

Function signature

For this student exercise, int getLine(char line[], int size); is OK - but curiously never defined?

In C, array indexes can exceed INT_MAX. The unsigned type size_t is size to accommodate all array indexing and object sizing. Consider

size_t getLine(char line[], size_t size); 
// or  
size_t getLine(size_t size, char line[]);

The first form is more common.
Advanced: The 2nd has an advantage when coded as

size_t getLine(size_t size, char line[size]);

Yet for various portability concerns, this needs to wait until C2x.

Must the last line contain a '\n'?

Your design choice.

Best to be clear about what code does if it received an EOF following a non-'\n'. That is, the last line did not end with a '\n'.

A text stream is an ordered sequence of characters composed into lines, each line consisting of zero or more characters plus a terminating new-line character. Whether the last line requires a terminating new-line character is implementation-defined. C17dr § 7.21.2 2

MAXLINE is not necessarily the same as LINECOUNT_N

Better as:

// char minCharsLine[MAXLINE][MAXLINE];

#define LINECOUNT_N 1024
char minCharsLine[LINECOUNT_N][MAXLINE];

MAXLINE versus alternates

Many off-by-one errors stem from not being clear.
MAXLINE sounds like the maximum returned strlen(line). To achieve that length, the array size needed is MAXLINE + 1.
Perhaps:

#define MAXLINE 80
#define LINE_SIZE (MAXLINE + 1)
char line[LINE_SIZE];

// or 

#define MAXLINE 80
char line[MAXLINE + 1];

Line counts can be large

Code performs minCharsLine[lineCount] without ever checking if lineCount is too high. Better to exit with an error message than go on.

The line count of a file is not limited to INT_MAX. I'd use unsigned long or uintmax_t for the line count. Of course even that is not the upper bound, just a safer bet.

Simplification

OP's code is OK and does handle the unexpected reading of null characters and "prints" them with:

        for (int j = 0; j < lineCharCount[i]; j++) {
            printf("%c", minCharsLine[i][j]);
        }

Simplier to use:

        fwrite(minCharsLine[i], lineCharCount[i], 1, stdout);

If code does not need to support reading null characters, make sure minCharsLine[i] is 1 wider, append a null character to minCharsLine[i],and use the below:

        fputs(minCharsLine[i], stdout);

Simplify??

Along time ago printf("%c", minCharsLine[i][j]) was considered inefficient versus the simpler putchar(minCharsLine[i][j]). That still applies to some compilers today, yet good compilers are smart and will emit efficient code either way.

Using putchar(minCharsLine[i][j]) is typically better, yet for such linear optimization cases as this over that, do consider clarity of code and let the compiler cope with optimizations.

fgets()

fgets() is mostly a drop in replacement for this exercise, yet it has functional difference in special cases.


*1 EOF and [0...UCHAR_MAX]. This may be more than 257 different values when UCHAR_MAX is more than 255.

\$\endgroup\$
4
  • 1
    \$\begingroup\$ Pedantry: s/257/UCHAR_MAX + 2/ \$\endgroup\$ Jan 6 at 12:59
  • \$\begingroup\$ @TobySpeight, answer amended. The pedantry gets uglier when UCHAR_MAX > INT_MAX, (e.g. all integer types are 64-bit). Such machines oblige code like int ch; fclear(stdin); while ((ch = getchar()) != EOF && (!feof(stdin) && !ferror(stdin))) { ... }. In that case, UCHAR_MAX + 1 different values. \$\endgroup\$ Jan 6 at 13:10
  • \$\begingroup\$ Oh that's ugly - I hope I don't have to deal with one of those... \$\endgroup\$ Jan 6 at 13:15
  • \$\begingroup\$ @TobySpeight For getline code, I have used while ((ch = fgetc(stdin)) != '\n' && !IS_TRUE_EOF(ch, stdin)) { and #if INT_MAX <= UCHAR_MAX #define IS_TRUE_EOF(ch, inf) ((ch)==EOF) #else #define IS_TRUE_EOF(ch, inf) (((ch)==EOF)) && (iseof(inf) || iserror(inf))) #endif to distribute the ugly. Its the prior fclear(stdin); I find especially troubling - I'd like to determine if EOF occurred due to an input error that just happened. \$\endgroup\$ Jan 6 at 13:27

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.