HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Program to replace tabs with blanks

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
blankswithreplaceprogramtabs

Problem

From my C university book 'The C Programming Language' by Brain W.Kernighan and Dennis M.Ritchie. Excercise 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.
Assume a fixed set of tab stops, say every
n columns. Should n be a variable or a symbolic parameter?

#include 

#define TABSTOP 8
#define MAXLINE 1000

int getline(char s[], int len);

int main()
{
    char line[MAXLINE];

    while ((getline(line, MAXLINE) > 0))
    {
        int i, j, count;

        count = 0;
        for (i = 0; line[i] != '\0'; ++i)
        {
            if (line[i] == '\t')
            {
                for (j = 0; j = TABSTOP)
                    count = 0;

                putchar(line[i]);
            }
        }
    }

    system("pause");

    return 0;
}

int getline(char line[], int len)
{
    int c, i;

    for (i = 0; i < len - 1 && (c = getchar()) != EOF && c != '\n'; ++i)
        line[i] = c;

    if (c == '\n')
    {
        line[i] = c;
        ++i;
    }

    line[i] = '\0';

    return i;
}


My concerns are that I didn't get the question right, did they mean that every time I see a tab in the input I need to put the corresponding amount of spaces instead of tabs?

Are there any flaws or bugs in the code?

I didn't understood what is a 'symbolic parameter', did they mean symbolic constant?

By 'n' did they mean what I did with the 'TABSTOP' symbolic constant?

Solution

Headers and Names

  • You are using the system function that is declared in stdlib.h but you don't #include .



  • The name getline is already taken by many C implementations for an extension to the standard library's I/O functions. You shouldn't use it for your own function names.



Program structure

Your program basically consists of the main function. While simple it is, this is not a very modular design and in particular hostile to unit testing. I suggest you decouple the I/O logic from the tab expansion logic. Have one function read the input line-by-line and for each line it reads, make it call another function that processes the line and places the results in a separate buffer. It could look like this:

size_t
expand_tabs(int tabwidth, const char * input, char * output, size_t length);


The first function then outputs that buffer. This way you can, for example, unit test expand_tabs with an array of strings.

Don't put

system("pause");


in your programs. I know many tutorials targeted at people using the Windows operating system suggest this but it is poor advice. If your program is done, make it exit. If this causes your terminal emulator to close the window before you wanted it to, fix the way you use the terminal emulator, not your program.
I/O Handling

Avoid character-based I/O. It's slow and painful. Instead of your hand-rolled getline function, you could have used the standard library fgets function. From its man page:

Synopsis

#include 

char *fgets(char *s, int size, FILE *stream);


Description

fgets() reads in at most one less than size characters from stream and stores them into the buffer pointed to by s. Reading stops after an EOF or a newline. If a newline is read, it is stored into the buffer. A terminating null byte ('\0') is stored after the last character in the buffer.

Return Value

fgets() returns s on success, and NULL on error or when end of file occurs while no characters have been read.

This is just what your getline function does but faster and cleaner.

Never use the seemingly simpler gets function. It is broken and C11 removed it from the standard library.

While using stack-allocated buffers like

char line[MAXLINE];


is fast, the hard-coded limit is a little awkward. You actually took care to make your program process files with lines longer than MAXLINE correctly too. Congratulations for that. Another option would be to use a dynamically allocated buffer. If your implementation has it, the aforementioned getline function comes in handy, here. If you are following the book, I'm not sure if you've already learned about dynamic memory yet, though.
Style

While declaring functions as

int my_getline(char s[], int len);


is valid syntax, it is misleading because the first argument actually is a pointer and not an array. So I prefer the (functionally equivalent)

int my_getline(char * s, int len);


syntax.

As long as you want to use MAXLINE to create an array, it needs to be a macro, unfortunately. For TABSTOP there is no such need. Avoid macros if possible. Here, a static const int would have been perfectly fine. Note that in the expand_tabs function suggested above, I'm even passing it as a parameter so it doesn't depend on global constants. Of course, the constant has to be coded at some point in your program.

Code Snippets

size_t
expand_tabs(int tabwidth, const char * input, char * output, size_t length);
system("pause");
#include <stdio.h>

char *fgets(char *s, int size, FILE *stream);
char line[MAXLINE];
int my_getline(char s[], int len);

Context

StackExchange Code Review Q#106883, answer score: 5

Revisions (0)

No revisions yet.