tags:

views:

470

answers:

4

I'm learning C from the k&r as a first language, and I just wanted to ask, if you thought this exercise was being solved the right way, I'm aware that it's probably not as complete as you'd like, but I wanted views, so I'd know I'm learning C right.

Thanks

/* Exercise 1-22. Write a program to "fold" long input lines into two or
 * more shorter lines, after the last non-blank character that occurs
 * before then n-th column of input. Make sure your program does something
 * intelligent with very long lines, and if there are no blanks or tabs
 * before the specified column.
 * 
 * ~svr
 *
 * [NOTE: Unfinished, but functional in a generic capacity]
 * Todo:
 * Handling of spaceless lines
 * Handling of lines consisting entirely of whitespace
 */

#include <stdio.h>
#define FOLD 25
#define MAX 200
#define NEWLINE '\n'
#define BLANK ' '
#define DELIM 5
#define TAB '\t'

int
main(void)
{
    int line  = 0, 
        space = 0,
        newls = 0,
            i = 0, 
            c = 0, 
            j = 0;

    char array[MAX] = {0};

    while((c = getchar()) != EOF) {
        ++line;
        if(c == NEWLINE)
            ++newls;
        if((FOLD - line) < DELIM) {
            if(c == BLANK) {
                if(newls > 0) {
                    c = BLANK;
                    newls = 0;
                }
                else
                    c = NEWLINE;
                line = 0;
            }
        }
        array[i++] = c;
    }
    for(line = 0; line < i; line++) {
        if(array[0] == NEWLINE)
            ;
        else
            printf("%c", array[line]);
    }
    return 0;
}
A: 

An obvious problem is that you statically allocate 'array' and never check the index limits while accessing it. Buffer overflow waiting to happen. In fact, you never reset the i variable within the first loop, so I'm kinda confused about how the program is supposed to work. It seems that you're storing the complete input in memory before printing it word-wrapped?

So, suggestions: merge the two loops together and print the output for each line that you have completed. Then you can re-use the array for the next line.

Oh, and better variable names and some comments. I have no idea what 'DELIM' is supposed to do.

zvrba
A: 

It looks (without testing) like it could work, but it seems kind of complicated.

Here's some pseudocode for my first thought

const int MAXLINE = ??  — maximum line length parameter
int chrIdx = 0 — index of the current character being considered 
int cand = -1  — "candidate index",  Set to a potential break character
char linebuf[bufsiz]
int lineIdx = 0 — index into the output line
char buffer[bufsiz]   — a character buffer
read input into buffer
for ix = 0 to bufsiz -1
do     
   if buffer[ix] == ' ' then
      cand = ix
   fi
   linebuf[lineIdx] = buffer[ix]
   lineIdx += 1
   if lineIdx >= MAXLINE then
      linebuf[cand] = NULL — end the string
      print linebuf
      do something to move remnants to front of line (memmove?)
   fi
 od

It's late and I just had a belt, so there may be flaws, but it shows the general idea — load a buffer, and copy the contents of the buffer to a line buffer, keeping track of the possible break points. When you get close to the end, use the breakpoint.

Charlie Martin
+7  A: 

I'm sure you on the rigth track, but some pointers for readability:

  • comment your stuff
  • name the variables properly and at least give a description if you refuse
  • be consequent, some single-line if's you use and some you don't. (imho, always use {} so it's more readable)
  • the if statement in the last for-loop can be better, like

    if(array[0] != NEWLINE)  
    {   
        printf("%c", array[line]); 
    }
Mafti
@Mafti: I'll follow all your suggestions; it was a first draft, just to see if I got the hang of it, in a slightly elegant manner. You're right about the last for-loop sequence, I probably should've known better to write it as such.Thanks
+2  A: 

That's no good IMHO.

First, it doesn't do what you were asked for. You were supposed to find the last blank after a nonblank before the output line boundary. Your program doesn't even remotely try to do it, it seems to strive for finding the first blank after (margin - 5) characters (where did the 5 came from? what if all the words had 9 letters?). However it doesn't do that either, because of your manipulation with the newls variable. Also, this:

for(line = 0; line < i; line++) {
    if(array[0] == NEWLINE)
        ;
    else
        printf("%c", array[line]);
}

is probably wrong, because you check for a condition that never changes throughout the loop.

And, last but not least, storing the whole file in a fixed-size buffer is not good, because of two reasons:

  • the buffer is bound to overflow on large files
  • even if it would never overflow, people still wouldn't like you for storing eg. a gigabyte file in memory just to cut it into 25-character chunks

I think you should start again, rethink your algorithm (incl. corner cases), and only after that, start coding. I suggest you:

  • process the file line-by-line (meaning output lines)
  • store the line in a buffer big enough to hold the largest output line
  • search for the character you'll break at in the buffer
  • then print it (hint: you can terminate the string with '\0' and print with printf("%s", ...)), copy what you didn't print to the start of the buffer, proceed from that
jpalecek