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

Simple pager in C

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

Problem

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

/*
* 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 th

Solution

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.

Context

StackExchange Code Review Q#111392, answer score: 3

Revisions (0)

No revisions yet.