4
\$\begingroup\$

This is my solution to K&R Ex 1-18 which requires us to remove all trailing blanks and spaces from a string. I specifically wanted to use a separate function for processing the input (via gtLine()) to promote code re-usability. Please tell me ways in which I could have made my program shorter/better. Also, please point out anything wrong that you might find with my programming logic or algorithm.

#include<stdio.h>
#define MAXLEN      100
#define STORELEN    1000

#define T   1
#define F   0

int gtLine(char[]);
int process(char[],int);
void append(char[],char[],int);

main()
{
    char line[MAXLEN],str[STORELEN];
    int len,pos;
    len=pos=0;

    while((len=gtLine(line))>0)
    {
        len=process(line,len);
        append(str,line,pos);
        pos+=len;
    }

    printf("\nThe text : %s\n" AC_RESET,str);
    return 0;
}

/*** Gets a line for Input from the user Till EOF or a new line is read. Returns the position of NULL ***/

int gtLine(char arr[])
{
    int c,i;

    for(i=0;(i<MAXLEN-1) && ((c=getchar())!=EOF) && (c!='\n');++i)
        arr[i]=c;
    if(c=='\n')
    {
        arr[i]=c;
        ++i;
    }
    arr[i]='\0';
    return i;       
}

/*** Processes the input string to remove all trailing spaces from the text. ***/
/*** Returns the new length of string                                        ***/

int process(char arr[],int len)
{
    int pc,nl;
    nl=F;                   // New line Flag
    if(arr[0]!='\n')
    {
        pc=arr[len-1];
        while(pc==' '||pc=='\t'||pc=='\n')
        {
            if(pc=='\n')
                nl=T;       // Set to 1 if at anypoint a new line character is encountered.
            --len;
            if(len!=0)
                pc=arr[len-1];
        }
        if(nl==T && len!=0)
            arr[len++]='\n';
        arr[len]='\0';
        return len;
    }
    else
    {
        arr[0]='\0';
        return (len=0);
    }
}

/*** Appends the string from[] to the end of to[] from to[pos] where pos= position of NULL in to[] ***/

void append(char to[],char from[],int pos)
{
    int i=0;

    while((pos<STORELEN) && ((to[pos]=from[i])!='\0'))
    {
        ++i;
        ++pos;
    }
    to[pos]='\0';
}
\$\endgroup\$
5
\$\begingroup\$

When working with strings, always try to avoid copying/duplicating/moving them, if possible. This would be a perfect example to do so.

When you think about strings in memory, they may be described as a specific number of characters (i.e. length or determined by a terminator) at a specific offset in memory. So all you'll have to do is move both, the starting offset as well as the end indicator (whether it's another pointer or some specific value, i.e. terminator, doesn't matter).

Both these tasks can easily be achieved using a loop in a single line each.

Here's what I'd do (simplified; try it on Ideone.com):

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

char *trim(char *text) {
    char *start = text; // points to the first character
    char *end = text + strlen(text); // points past the last character

    // Let's start with the left side.
    // Move the pointer as long as there's a whitespace
    for (; isspace(*start); ++start);

    // In a similar way, let's get the right end.
    // Again, move the pointer as long as there's a whitespace right before it
    for (;end > start && isspace(end[-1]); --end);

    // Insert the new terminator
    *end = '\0';

    // Return the new starting pointer
    return start;
}

int main(int argc, char **argv) {
    char string[] = "  \t  Hello World\t\t\n";
    printf("result: '%s'\n", trim(string));
    return 0;
}

Note that I'm removing line breaks as whitespaces as well. In this example you'd add it after trimming the line.


Other points regarding your code:

  • If this is no contest to write code that's as short as possible (like code golf), try to use telling names. len is good, arr is good as well, but pc and nl aren't (nl is obviously "new line", but how about pc?).

  • The same applies to your preprocessor constants. This is even more important, since you might unintentionally rewrite code. As such never define F or T. If you want a TRUE and FALSE, call them like that or simply use 0 or 1.

  • In a similar way, use white spaces to make your code more readable.

    For example, the line:

    while(pc==' '||pc=='\t'||pc=='\n')
    

    could be reformatted to this:

    while (pc == ' ' || pc == '\t' || pc == '\n')
    

    This way it's easier to see where single expressions start or end, especially combined with syntax highlighting colors.

  • I'd also rewrite the contents of your append() function. Since there's nothing happening inside the loop other than the increments, you could use a for() instead:

    void append(char to[], char from[], int pos)
    {
        int i;
    
        for (i = 0; pos < STORELEN - 1 && from[i]; to[pos++] = from[i++]);
    
        to[pos] = '\0'; // Only required in case you run out of space
    }
    

    As long as your target string isn't too long already and there's still a character left in your source string it will copy the contents and then move both offsets every iteration. Also note the - 1 when checking for the maximum length. Otherwise pos would point one past the maximum length (see below).

  • There's a buffer overrun in your code:

    • If you define char string[1000], your last valid offset would be 999, since the index starts at 0.

    • When checking pos < STORELEN and this check fails, the value of pos would be 1000 already. Once you hit the line to[pos] = '\0' you're essentially writing past the allocated memory at string[1000].

\$\endgroup\$
  • \$\begingroup\$ Thank you for all the hard work you've put into your reply! I should mention that I'm currently solving the problems in K&R's the C Prog Language book, and am trying to only use the features described in the book so far, and hence pointers are out of the question. \$\endgroup\$ – Somu Apr 29 '15 at 12:09
  • \$\begingroup\$ @Somu I know. However, you're working with pointers already, even if you don't know. :) arr[pos] is the same as *(arr + pos) - syntactic sugar. \$\endgroup\$ – Mario Apr 29 '15 at 12:12
  • \$\begingroup\$ Nice trim() function - even works with "". Very complete answer -hard to add anything worthy of a completing answer. Pedantic issue: isspace(*start) may be UB when char is signed. Suggest isspace((unsigned char) *start). BTW: Could do char *end = start + strlen(start); after for (; isspace(*start); ++start); \$\endgroup\$ – chux May 1 '15 at 3:05

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.