3
\$\begingroup\$

So far I have reached chapter 5 of the K&R Book, Edition 2 from which I have been learning C. I spent a few days thinking of a solution to this program. I wrote two or three versions of the same program, And the truth is that I couldn't bring myself to think of a better solution for this exercise:

Exercise 5-11. Modify the program "entab" and "detab" (written as exercises in Chapter 1.) to accept a list of tab stops as arguments. Use the default tab settings if there are no arguments.

And well, I assumed that this was the best one I had written. Here is the solution for the exercise (Chapter 5, Ex-5.11):

I would like to know how to improve it to take it "further?" or if I'm taking the right approach:

/*-
 *      detab.c - expand tabs to equivalent spaces
 *
 *      The-C-Programming-Language Book 2nd Edition.
 *
 *      Created by jr.chavez on 9/22/22.
 */
#include <stdio.h>
#include <stdlib.h>

#define DTAB_STOP       8       /* DEFAULT TAB STOP */
#define MAX_TAB_STOP    100

enum escapes { BACKSPACE = '\b', TAB = '\t', NEWLINE = '\n', RETURN = '\r' };

static int
getstops(char *cp)
{
        int n;
        
        n = 0;
        while (*cp) {
                if (*cp < '0' || *cp > '9')
                        return -1;
                if (*cp >= '0' && *cp <= '9')
                        n = n * 10 + *cp - '0';
                cp++;
        }
        return n;
}


int
main(int argc, char *argv[])
{
        int ch;
        int n, col;        /* n, columns */

        int nstops;
        int tabstops[MAX_TAB_STOP];

        nstops = argc - 1;
        if (nstops >  MAX_TAB_STOP) {
                fprintf(stderr, "error: too many args.\n");
                exit(EXIT_FAILURE);
        }
        for (int indx = 1; indx < argc; indx++) {
                tabstops[indx - 1] = getstops(argv[indx]) - 1;
        }

        col = 0;
        while ((ch = getchar()) != EOF) {
                switch (ch) {
                case TAB:
                        if (nstops == 0) {
                                while (col < DTAB_STOP) {
                                        printf("\u25A0");
                                        col++;
                                }
                                continue;
                        }
                        for (n = 0; n < nstops; n++)
                                if (col < tabstops[n])
                                        break;
                        if (n == nstops) {
                                printf("\u25A0");
                                col++;
                                continue;
                        }
                        while (col < tabstops[n]) {
                                printf("\u25A0");
                                col++;
                        }
                        break;
                case BACKSPACE:
                        if (col > 0)
                                --col;
                        putchar(BACKSPACE);
                        break;
                case RETURN:
                case NEWLINE:
                        putchar(ch);
                        col = 0;
                        continue;
                default:
                        putchar(ch);
                        break;
                }
        }
        if (ferror(stdin)) {
                perror("stdin");
                return EXIT_FAILURE;
        }
        return 0;
}

I did some "tests":

cat test.txt | ./bin/detab 4 9 29

Input File:

#           #       #
#               #       #
#           #   #           #   #
#   #           #

Output:

#■■■■■■■■■■■■■■■■■■■■■■■■■■■■#■■#                                                                                                 
#■■■■■■■■■■■■■■■■■■■■■■■■■■■■■#■■#                                                                                                
#■■■■■■■■■■■■■■■■■■■■■■■■■■■■#■#■■■#■#      
#■■■#■■■■■■■■■■■■■■■■■■■■■■■■■■#

These are the results I get with the expand command: (Of course, I never intended the results to be accurate in the first place - but for now I'd just like to improve it.)

cat test.txt | expand -t 4,9,29

output:

#                            #  #
#                             #  #
#                            # #   # #
#   #                         #

Note:

  • I've only written the detab program, for now.

  • I had focused on doing something "similar" to what the "expand" command does. (I don't get the same output, but something similar). I also didn't add flags or read from a file. (I still haven't reached Chapter 7.).

\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

how to improve it to take it "further?"

User input is evil.

getstops(char *cp), which should be getstops(const char *cp) does not detect invalid conversions. Better to use strtol() and test for acceptable range. Error out on bad input.

Pedantic: Hacker hardening

getstops(argv[indx]) - 1; invokes undefined behavior when getstops() returns INT_MIN. Obviously this is not an expected return value, yet better to test valid range of user values before using them in calculations.

Black square

Rather than printf("\u25A0"); consider #define BLACK_SQUARE "\u25A0", fputs(BLACK_SQUARE, stdout); or the like.

More informative message

// fprintf(stderr, "error: too many args.\n");
fprintf(stderr, "error: More than %d args.\n", MAX_TAB_STOP + 1);
// or 
fprintf(stderr, "error: More than %d tab stops.\n", MAX_TAB_STOP);

Good error detection

Nice: if (ferror(stdin)) {

Be generous

I could see automated tools making many tap stops.

// #define MAX_TAB_STOP    100
#define MAX_TAB_STOP    4096

Better code would simple allow argc tab stops. Size int tabstops[] to argc.

Useless code

if (*cp >= '0' && *cp <= '9') serves no purpose. It is already known cp is in range.


#include <stdlib.h>

// Parse for positive int
static int getstops(const char *cp) {
  errno = 0;
  char *endptr;
  long val = strtol(cp, endptr, 0);
  if (cp == endptr || *endptr) {
    fprintf(stderr, "Non-numeric conversion <%s>\n", cp);
    exit(EXIT_FAILURE); // or alternative handling
  }
  if (errno || val < 0 || val > INT_MAX) {
    fprintf(stderr, "Invalid tab stop %ld>\n", val);
    exit(EXIT_FAILURE); // or alternative handling
  }
  return (int) val;
}
\$\endgroup\$
3
\$\begingroup\$

General Observations

The code is mostly readable.

Command lines may contain flags so the command line processing should check for them.

There may be more error conditions that you aren't checking for.

Some of the variables are excellent such as tabstops, variable names such as n, col and cp can be improved.

Complexity

This was covered by part of another answer by luser droog in your last question.

The function main() is around 60 lines of code, depending on the editor or IDE you are using this may be too many lines of code. Functions that can't fit into one screen in an editor are generally considered too large. It gets very difficult to maintain a function that you can't see all at once.

The function main() is too complex (does too much). As programs grow in size the use of main() should be limited to calling functions that parse the command line, calling functions that set up for processing, calling functions that execute the desired function of the program, and calling functions to clean up after the main portion of the program.

There is also a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states:

that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

There are at least 3 possible functions in main().

  • Process the command line
  • Get the input
  • In the switch statement the code to process tab should be a function.

Use of the exit(status) Function

Inside the main() function there is no reason to call the exit() function. The exit function should only be called in functions that are not executed by the operating system.

Instead of

                exit(EXIT_FAILURE);

the code should be

                return EXIT_FAILURE;
\$\endgroup\$
2
  • \$\begingroup\$ I guess the same applies when lines of code are very long. Thank you I will take it into consideration from now on. PS: I think I should study a bit about time & complexity (Big O Notation) \$\endgroup\$ Commented Sep 25, 2022 at 0:57
  • 1
    \$\begingroup\$ @jr.çhåvez__07 This complexity is different than Big O Notation, but you will only understand that by looking into it. Good choice on the accepted answer. \$\endgroup\$
    – pacmaninbw
    Commented Sep 25, 2022 at 14:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.