Simple pager in C

Question

I have written a simple pager in C. It works fine for most things, but I'd like to know how it can be improved. Specific things I'm looking for are if there are size and/or speed improvements to be made, as I have noticed that it sometimes performs slowly (but that might be my slow computer ;-), and usability. This obviously uses (n)curses.

#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <string.h>
#include <curses.h>
#include <stdbool.h>
/*
 * some.c - A small and simple pager written by Daniel Roskams
 */
void view_file(char *, int, int);
/*
 * A function to view files. This does most of the work here. It returns on
 * error.
 */
void view_file(char *file, int row, int col)
{
 int fch; /* file character */
 int uch; /* user character */
 int x; /* not used currently */
 int y;
 FILE *fp;
 (void) col; /* keep the compiler happy */
 if (strcmp(file, "-") == 0) {
 fp = stdin;
 } else if ((fp = fopen(file, "r")) == NULL) {
 perror(file);
 endwin();
 printf("\n");
 return; /* error */
 }
 while ((fch = fgetc(fp)) != EOF) {
 /* Get current x/y coordinates of cursor */
 getyx(stdscr, y, x);
 (void) x;
 /* if we printed a screenful of stuff */
 if (y == (row - 1)) {
 refresh();
 attron(A_BOLD);
 printw("[return or spacebar to advance, q to quit]");
 attroff(A_BOLD);
 for (;;) {
 uch = getch();
 switch (uch) {
 case 'q':
 fclose(fp);
 endwin();
 putchar('\n');
 return;
 case '\n':
 case ' ':
 break;
 default:
 continue;
 }
 break;
 }
 /* Clear the screen and go to the beginning */
 clear();
 move(0, 0);
 } else {
 /* Print out the character */
 printw("%c", fch);
 }
 }
 (void) getch();
 if (fp != stdin) {
 fclose(fp);
 }
 return;
}
int main(int argc, char *argv[])
{
 int row, col;
 int i;
 /* initialise curses */
 initscr(); /* start curses mode */
 noecho(); /* don't echo typed stuff */
 raw(); /* don't generate escape chars */
 /* make everything reverse video-ish */
 if (has_colors()) {
 assume_default_colors(COLOR_BLACK, COLOR_WHITE);
 start_color();
 }
 getmaxyx(stdscr, row, col); /* find screen size */
 if (argc < 2 && !isatty(fileno(stdin))) {
 view_file("-", row, col);
 }
 for (i = 1; i < argc; i++) {
 view_file(argv[i], row, col);
 }
 endwin();
 return 0;
}

Something nice to note is that this compiles with no warnings using this command:

$ clang -Oz -Weverything -o some some.c -lcurses

• glampert: Your compiler command line is enabling optimization for code size (-Oz), which is rare unless you really have a code size constraint. If not, the compiler will be able to generate faster code with -O3.
• glampert: -O3 is the highest available not targeting smaller code size. Run man clang for a list of all flags and a brief description of each. They also have specific optimization switches that might be of interest for your target system.
• chux: Sure int row, col; ... getmaxyx(stdscr, row, col); works? In C, I would expect something like getmaxyx(stdscr, &row, &col);
• user1118321: @chux - I found this page which says: "All of these interfaces are macros. A "&" is not necessary before the variables y and x." It's a terrible interface because it looks like something it's not, but apparently that's how it works.
• DrProgrammer: @AnonymousShadow void view_file(char *, int, int); is not necessary in that case if you prefer having forward declarations before the entry point and implementation after it.

Answer 1

Overall, I'd say this code seems pretty straightforward and is easy to read. My main concern is the performance. You're reading a file in one character at a time and that's going to be slow.

Rather than using fgetc(), I recommend using getline() instead. It's a standard function that reads an entire line at a time.

The other concern I have is what happens when a line is longer than the width returned by getmaxyx()? Does it wrap? Does it overwrite the last character? Does it just not print because it's off screen?

Whatever it does, it seems like you should be handling it in some way, or at the least document that you want the standard behavior, and what that behavior is. If someone updates this to use something other than curses in the future, they may want to keep the same behavior, which might not be the default in this hypothetical new library.