8
\$\begingroup\$

These are 3 functions I created in C language to read in a text file, remove the carriage returns from it in order to display on the screen, and display the file on screen wrapped with the words of the text not breaking (this is the most important task of the program: wrapping words without breaking them). I created a simple driver for testing the functions. I chose the name "msed" as the program name.

I intend to create a simple text editor in the near future using functions I create in order to learn and practice C programming.

I created the program and compiled it on cygwin under windows using ncurses. Any opinions, corrections, suggestions will be greatly appreciated.

You can run the program by entering the following at the cygwin terminal: ./msed.exe filename.txt

#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <curses.h>
#include <stdlib.h>      /* has the malloc prototype*/
#include <string.h>

#define BUF_MAX 100000


struct line
    {
    char line_buf[BUF_MAX]; //each text line has its own buffer in a linked list.
    struct line * next;
    };

void fill_list(FILE * fptr, struct line ** head, struct line ** prev, struct line ** current);
void rem_cr(struct line **head, struct line **current);
void display(struct line **head, struct line **current);

//the driver
int main(int argc, char * argv[])
{
    FILE * fptr;
    struct line * current;
    struct line * prev;
    struct line * head = NULL;
    initscr();
    noecho();
    cbreak();
    keypad(stdscr, TRUE);
    erase();
    refresh();
    fptr = fopen (argv[1], "r");
    fill_list(fptr, &head, &prev, &current);
    rem_cr(&head, &current);
    display(&head, &current);
    sleep(10);
    fclose(fptr);               
    endwin();
    return 0;
}

//fill_list function take each line ending with \n from the text file and put it in its own buffer.
void fill_list(FILE * fptr, struct line ** head, struct line ** prev, struct line ** current)
{
    char buffer[BUF_MAX];
    while (fgets(buffer, BUF_MAX-1, fptr) != NULL)
    {
        *current = (struct line *) malloc(sizeof(struct line));
        if (*head == NULL)       // first structure       
            *head = *current;
        else                    // subsequent structures                        
            (*prev)->next = *current;
        (*current)->next = NULL;
        strcpy((*current) -> line_buf, buffer);
        *prev = *current;
    }
}

// rem_cr function removes carriage returns as this should be done to display the text correctly on the     terminal.
void rem_cr(struct line **head, struct line **current)
{
    char * src;
    char * dst;

    *current = *head;

    while (*current != NULL)
    {

        for (src = dst = (*current)-> line_buf; *src != '\0'; src++)
        {
            *dst = *src;
            if (*dst != '\r') dst++;
        }
        *dst = '\0';
        *current = (*current)->next;
    }
}

// display function displays lines of text wrapped (no word breaks) (this is the most imporant function I think and I wish you direct me to how I can make corrections (if any) and improve it and your opinions will be appreciated.)
void display(struct line **head, struct line **current)
{
    int i=0;
    int x=0;
    int y=0;
    int j;
    int cpw; //characters per word
    char *buf;

    *current = *head;

    while(*current != NULL)
    {
        buf=(*current)->line_buf;
        while(buf[i]!= '\n')
        {
            if(buf[i]=='\0')
                return;
            while(buf[i]==' ')
            {
                if(x>COLS-2)
                {
                    x=0;
                    y++;
                }
                move(y,x);
                addch(buf[i]);
                refresh();
                x++;
                i++;
            }
            j=i;
            cpw=0;
            while(buf[j]!=' '&&buf[j]!='\n'&&buf[j]!='\0')
            {
                cpw++;
                j++;
            }
            if(buf[j]==' ')
                cpw++;
            if(cpw+x>COLS-1)
            {
                x=0;
                y++;
            }
            while(buf[i]!=' '&&buf[i]!='\0'&&buf[i]!='\n')
            {
                if(x>COLS-2)
                {
                    x=0;
                    y++;
                }
                move(y,x);
                addch(buf[i]);
                refresh();
                x++;
                i++;
            }
            refresh();
        }
        *current = (*current)->next;
        x=0;
        y++;
        i=0;
        refresh();
    }
    refresh();
}
\$\endgroup\$
5
\$\begingroup\$

Your display function's logic is very hard to follow, there are a lot of special cases. Some comparisons are with COLS-1, others with COLS-2, and it's not clear why. The variable names don't help much here. The last line displayed is odd too, you're not checking for "vertical overflow".

You call refresh and manually move the cursor around for each character output (making you track both x and y all over the place) which isn't efficient.

And the arguments passed in aren't good: why pass in current at all, and why pass a pointer-to-pointer for the head when that function should not modify it?

I'd suggest three things:

  • change the arguments to only take struct line *head
  • move the line-break detection out of the display function
  • only refresh the screen once when all the output is done.

For that, here's a helper function that finds the right place to break in a string given a start position (where the previous line was broken off) and a width:

int find_break(char * text, int start, int width)
{
  int cur = start;
  int last_break = -1;
  while ((cur - start < width) && text[cur] != '\n' && text[cur] != 0) {
    if (text[cur] == ' ')
      last_break = cur;
    cur++;
  }
  if (cur - start < width)
    return cur;
  return last_break;
}

Essentially it goes as far as it can in the input string, recording the last time it saw a space. If it reached the end of string (newline or '\0') without going over the width limit, returns the end of string. Otherwise, returns the position of the last space. (And if it couldn't find a break position and overflowed the width, returns -1.)

Then your display function could be re-written like this (I've left a TODO in there):

void display(struct line *head)
{
  int row = 0;
  struct line *current = head;

  while (current != NULL) {
    char *buf = current->line_buf;
    int start = 0;
    int end;

    while ((end = find_break(buf, start, COLS)) != -1) {
      // output current input line piece
      while (start < end) {
        addch(buf[start]);
        start++;
      }
      // move to next screen line, if possible
      row++;
      if (row >= LINES) {
        refresh();
        return;
      }
      move(row, 0);
      // check if we're done with this input line
      if (buf[end] == '\0' || buf[end] == '\n')
        break;
    }
    if (end == -1) {
      // word too long to fit on screen
      // what to do?
      refresh();
      return;
    }
    current = current->next;
  }
  refresh();
}

With this change, you'll also notice that you could get rid of the whole list thing and pass in the file pointer to display, saving a bunch of allocations (and the related leak). The \r special handling can be moved to find_break for not much effort.


Other things:

*current = (struct line *) malloc(sizeof(struct line));

Don't cast the result of malloc in C.

void fill_list(FILE * fptr, struct line ** head, struct line ** prev, struct line ** current)
void rem_cr(struct line **head, struct line **current)

Same as for your display functions, the parameters aren't good. fill_list should take only a FILE *, and return a struct line *. The prev and current should be local variables. rem_cr should just take a struct line*. I haven't really looked at the implementations of both of these, but simply making those changes should also make their code simpler and easier to read.

You're missing a free_list function, so you're leaking potentially lots of data.

You're not checking if the program got any arguments, so you get undefined behavior if the user doesn't pass a file name. Same thing if the user passes an incorrect file name or gives a file they don't have read permission on - you need to check the return value of all system calls.

struct line
    {
    char line_buf[BUF_MAX];
    struct line * next;
    };

I like your coding style, and you've applied it consistently which is good. But why the fancy indent on the braces here? I would un-indent those braces.

| improve this answer | |
\$\endgroup\$

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.