3
\$\begingroup\$

I am trying to finish the K&R book on C. Below is an exercise which took a long time to finish. I would like some feedback on optimization or any blatant issues with the code.

Exercise 1-21

Write a program entab that replaces strings of blanks by the minimum number of tabs and blanks to achieve the same spacing. Assume a fixed number of tabstops.

#include<stdio.h>
#define TABSTOP   8
#define TABCHAR   '\t'
#define SPACECHAR '#'

void entab(char input[]);
void printchars(char c, int times);
void printruler();

int main(int argc, char const *argv[])
{
  /*char input[] = "this         is an   aw some piec  of code.\n";*/
  char input[] = "this         is an   aw some piec  of code.\nright o            bro!\n";
  printruler();
  printf("%s", input);
  entab(input);
  printruler();
  return 0;
}


void entab(char input[])
{
  int c;

  for (size_t i = 0, pos = 1, spaces = 0; (c = input[i]) != '\0'; ++i){

    if (c == ' '){
      //else increment the number of spaces
      ++spaces;

      //if we have spaces equal to 1 tab width print the tab
      if(pos % TABSTOP == 0){
        putchar('\t');
        spaces = 0;
      }

    } else{
      //if the current character is not a space print the spaces and the character
      printchars(SPACECHAR, spaces);
      putchar(c);
      spaces = 0;
    }

    if( c == '\n' )
      pos = 1;
    else
      pos += 1;

  }
}


void printchars(char c, int times){
  for (size_t i = 0; i < times; ++i){
    putchar(c);
  }
}


void printruler(){
  for (int i = 1; i < 100; ++i){
    if(i%TABSTOP == 0)
      putchar('|');
    else
      putchar('_');
  }
  putchar('\n');
}
\$\endgroup\$
  • \$\begingroup\$ Small comment. It does not handle strings with spaces and tabs already intermixed correctly. "A \tB" Should be encoded as "A\tB" but it is not. \$\endgroup\$ – Martin York Feb 15 '12 at 17:00
  • \$\begingroup\$ I think you're misusing size_t where you should be using int. \$\endgroup\$ – Ant Feb 15 '12 at 17:08
  • \$\begingroup\$ @Ant: I don;t think there is misuse here (though I would also use int in most situations (but size_t will make sure it works for very long strings)). size_t just means it will be non negative \$\endgroup\$ – Martin York Feb 15 '12 at 17:34
  • \$\begingroup\$ @Loki: Take a look at the signature for printchars(). It accepts times as an int and then iterates to that value using size_t; at least one of those is incorrect. \$\endgroup\$ – Ant Feb 15 '12 at 18:09
  • \$\begingroup\$ @Ant: Good point. Create an answer below and I will vote that up. Being consistent with types is important. \$\endgroup\$ – Martin York Feb 15 '12 at 18:59
2
\$\begingroup\$

Be consistent with you style:
In yout first two function you use the style of putting the brace on the next line.

int main(int argc, char const *argv[])
{

void entab(char input[])
{

While the second two functions you use the style of the trailing '{'

void printchars(char c, int times){

void printruler(){

People can have big religious wars about the two. Personally I prefer the first one in my code but its not a big deal if a coding standard says use the second one (I don't care enough to argue about it). BUT everybody agrees that consistency is important so pick one and stick to it.

Also note the second style of putting the '{' is usually only used for non function scope blocks and that most people use the '{' on the next line for functions (but this is a style thing so not a big deal). Just want to emphasis that consistency is the key.

If you are going to claim that it is aw some (then make sure you have spell checking turned on)

  char input[] = "this         is an   aw some piec  of code.\nright o            bro!\n";

In the function entab:
You don't detect and thus compensate for '\t' characters in the middle of a string of spaces

Thus "A \tB" is encoded as "A \tB" it should be encoded as "A\tB"

In C (unless you are explicitly using C99) you should not use // style comments. A lot of C compilers are not C99 compliant but a lot are not C99 compliant and support //. You need to be careful on this usage if you want to be portable.

Then is easier written as:

    if( c == '\n' )
      pos = 1; 
    else
      pos += 1;

    // In my opinion this is clearer (though to beginners it may not be)
    // The intent is to make sure pos is defined (this is not clear with an if
    // unless you read the whole conditional).
    pos = (c == '\n') ? 1 : pos + 1;

Avoid not using braces. It may seem like a waste of time. But get used to using them they will save you from bugs now and then and they do no harm when not needed.

    if( c == '\n' ) {
      pos = 1;
    } 
    else {
      pos += 1;
    }

Same thing below:

    if(i%TABSTOP == 0)
      putchar('|');
    else
      putchar('_');

    // Or
    putchar( (i%TABSTOP == 0) ? '|' : '_' );

This is also a perfect example of where is dangerous not to use '{}' around the conditional blocks. putchar is not actually a function call (even though it looks like one). Its a macro that will be expanded inline (luckily here putchar is so commonly used that it is written well). But in C there are a lot of macros that look like functions and if they are not well written they will screw up what looks like perfectly good code.

\$\endgroup\$
  • \$\begingroup\$ Thank you for such a detailed response, learnt a lot from it. I am using the c99 flag for my compiler. \$\endgroup\$ – Khaja Minhajuddin Feb 16 '12 at 4:27
3
\$\begingroup\$

Be careful to keep your types consistent. Here's your printchars() method:

void printchars(char c, int times){
  for (size_t i = 0; i < times; ++i){
    putchar(c);
  }
}

You're expecting times to be passed as an int, but then you treat it like a size_t in the for loop. You should choose one type and stick with it.

Personally I think you should be using int rather than size_t to ensure your that types reflect their usage and make your code somewhat self-documenting.

\$\endgroup\$
2
\$\begingroup\$

Bugs

  • If a line ends with spaces, those spaces get discarded. That might be tolerable for many purposes, but you never know when trailing spaces might be significant, as in format=flowed e-mail (RFC 3676 §4.1).
  • If the input already contains tab characters, you interpret them as a single-unit-width characters rather than advancing to the next tab stop.

Interface

  • Your function prints the transformed text to standard output. For greater flexibility and reusability, it should stick to transforming the text; this is known as the single-responsibility principle. Since the output will be no longer than the input, it would be convenient to write the result into the same buffer that contained the input. This is acceptable practice in C, as long as you document the fact that it's an in+out parameter. (I would also return the length of the resulting string.)
  • The tab stop width could be parameterized, and therefore probably should be.

Style

  • There's a lot going on in the for-loop header. I would move the initializers for pos and spaces out of the loop, since they don't participate in the condition and update portions of the loop. With that change, the loop is mainly just about i, with the side-effect of setting c.
  • It's common to define helper functions first and main() last, so that you don't have to pre-declare your functions.
  • I suggest renaming pos to col for clarity, because pos might be an array index, whereas col is definitely a column number on screen.

Here's what I came up with:

/**
 * Replaces spaces with tab characters in buf such that the visual layout
 * is retained.  The result is written in buf, and the length of the resulting
 * string is returned.
 */
size_t entab(char buf[], int tabstop) {
    size_t j = 0;
    char c;
    int col = 0, spaces = 0;

    for (size_t i = 0; (c = buf[i]) != '\0'; ++i) {
        col++;

        switch (c) {
          case ' ':
            ++spaces;

            /* We have enough spaces to reach a tab stop, so output a tab. */
            if (col % tabstop == 0) {
                buf[j++] = '\t';
                spaces = 0;
            }
            break;

          case '\t':
            spaces = 0;
            col += tabstop - (col % tabstop);
            buf[j++] = c;
            break;

          case '\n':
            col = 0;
            /* Flow through... */
          default:
            /* Flush any buffered spaces */
            while (spaces > 0) { buf[j++] = ' '; spaces--; }
            buf[j++] = c;
        }
    }
    while (spaces > 0) { buf[j++] = ' '; spaces--; }
    buf[j] = '\0';
    return j;
}
\$\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.