2
\$\begingroup\$

Task (1.20)

Write a program detab that replaces tabs in the input with the proper number of blanks to space to the next tab stop.

Also, the condition is is not to use pointers and structs etc because they will be considered in next chapters.

I'm new in C, and want to know what in my solution is bad idea and what can be done other way (but without pointers):

#include <stdio.h>
#define TABLINE 4

int count(char stri[]);

void main() {

    printf ("\nTabline is %d\n", TABLINE);

    int i = 0;
    int h;
    char stri[100];
    char c;

    while ((c = getchar()) != '\n' && c != EOF) {
        if (c == '\t')
            for (h=0; h<TABLINE; ++h) {
                stri[i] = ' ';
                ++i;
        } else {
            stri[i] = c;
            ++i;
        }
    }

    printf ("%d\n", count(stri));

    for (i=0; i< count(stri); ++i)
        printf ("%c", stri[i]);
}

int count(char stri[]) {

    int l;

    for (l=0; stri[l]; ++l)
        ;

    return l;
}

And its work:

$ ./detab

Tabline is 4
befortan        aftertab
20
befortan    aftertab
\$\endgroup\$
2
  • 3
    \$\begingroup\$ Your code is not correct. Replacing tabs is not as easy as replacing a tab with 4 spaces. The number of spaces is variable and depnds on where the next tab stop is. You have to place the number of spaces required to reach the next tab stop (otherwise you ruin in the indentation of the input). \$\endgroup\$ Mar 28 '15 at 20:43
  • 2
    \$\begingroup\$ Also you should do it for a whole file not just one line. \$\endgroup\$ Mar 28 '15 at 20:45
3
\$\begingroup\$

Apart from the fact that a tab doesn't necessarily map to 4 spaces, as was nicely put by @Loki in his comment, there are a few other points that you can improve:

  • Your main function is not following a standard signature. It should be returning an int.

  • Your count() function, besides having a vague name (what is it counting exactly?), is reinventing strlen(). Avoid reinventing the wheel, unless you have a good reason to (in this case I'd say yes, since you are learning, but it is also valuable to know the standard functions and how to use them).

  • Still talking about count(), avoid calling it in the conditional section of a loop, like here:

     for (i=0; i< count(stri); ++i)
    

    Remember that the condition gets tested every iteration of the loop, so this would imply a full scan of stri if the compiler is not smart enough to hoist that function call out the loop. Better would be to call the function once and store its result in a local variable.

  • Declare variables as close to their first usage as it is possible. For instance, h could be declared inside the body of the for loop:

    for (int h = 0; h < TABLINE; ++h)
    

    Reducing scope is important to avoid name collisions and facilitate the understanding of the program states. If you had to remember the state of all variables in a program, you would be doomed. Variables that are scoped inside a { } block can be forgotten as soon at the block ends. There is a discussion here that might also find relevant.

  • You make two other dangerous assumptions in your program: One is that the input will never exceed one line (you terminate the loop in the first '\n') and that the input will never exceed 100 characters (that's the size of your string buffer). Those assumptions are very unrealistic. More than likely the input would consist of multiple lines in a real world scenario. A 100 chars limit is also quite small. If using a statically allocated buffer, without resorting to malloc and pointers, at least make it a bit bigger and make sure to end the loop when the buffer is full. Something in the lines of (where MAX_CHARS is the size of your char[] buffer):

    while (i < MAX_CHARS && (c = getchar()) != '\n' && c != EOF) {
        ...
    }
    
  • Lastly, there is a confusion with the curly braces in the main loop which I'm not quite sure how the compiler is resolving, but it certainly is broken:

    while ((c = getchar()) != '\n' && c != EOF) { 
        if (c == '\t')  
            for (h=0; h<TABLINE; ++h) { 
                stri[i] = ' ';
                ++i;
        } else { // <--- does this belong to the first if ???
            stri[i] = c;
            ++i;
        } 
    } 
    

    As you can see if you look carefully, the } else part is bound to the { in the for loop, the if has no {. I was surprised that this compiled, but it did!

    Be very careful with this. My advice is to always add curly braces on all control statements and loops, even for the single line ones.

\$\endgroup\$
5
  • \$\begingroup\$ Thanks for exellent comment. Could you please clarify few moments? > "Declare variables as close to their first usage as it is possible" - does it mean - I must declare them everywhere in such way? In while {} for c, in for {} for h, in for {} for l? And also about > "end the loop when the buffer is full" - could you show small example - how t can be done (without finctions from stand. lib and/or pointers)? \$\endgroup\$
    – setevoy
    Mar 29 '15 at 6:33
  • \$\begingroup\$ > the } else part is bound to the { in the for loop, the if has no { // from "Ice age" - "Never say "I'm wrong", better to say "Wow - how interesting happened!" :-) But - yes, you right, there is really worng with {}... \$\endgroup\$
    – setevoy
    Mar 29 '15 at 6:55
  • 1
    \$\begingroup\$ @setevoy, The for is a special case where you can declare a variable inside the loop, you can't do that with while. I've expanded the answer on those two points, take a look and let me know if you need further clarification. \$\endgroup\$
    – glampert
    Mar 29 '15 at 16:21
  • \$\begingroup\$ Thanks, got it. Just one note more: I have compilation error: ‘for’ loop initial declarations are only allowed in C99 mode and use option -std=c99 or -std=gnu99 to compile your code - is it OK? Just add -std=c99 arg every time? It not looks good, as for me... (gcc version 4.4.7 20120313 (Red Hat 4.4.7-11) (GCC)) \$\endgroup\$
    – setevoy
    Mar 29 '15 at 18:27
  • 1
    \$\begingroup\$ @setevoy, humm, yes, it seems you compiler is configured to default to an older C standard. I think you should target C99 and above, the older standards are quite outdated and only used when you need to maintain legacy code. Add the -std=c99 flag and you should be good to go ;) \$\endgroup\$
    – glampert
    Mar 29 '15 at 19:11

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.