A program that reads input until end-of-file and echoes it to the display

Question

It echoes files or strings to output, never both at once. It does work (in both Linux and Windows), but I can NOT guarantee that it is bug free. It works much like echo, but it works with files as well, and has fewer formal arguments.

I'm looking for helpful and useful critiques, mainly in programming style, organization, and if it's easily understood (or if there was difficulty in understanding) source code. If any critiques are made, please add in how I can improve upon my "misdeeds".

//Problem 11-15
/*
*************************************************************
CHP 11 Problem #15
*************************************************************
Write a program that reads input until end-of-file and echoes 
it to the display. Have the program recognize and implement 
the following command-line arguments:
*************************************************************
-p -- Print input as is
-u -- Map input to all uppercase
-l -- Map input to all lowercase
*************************************************************
*/
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <ctype.h>
#define TITLE "Chapter 11 Exercise 13"
_Bool true = 1, false = 0;
void display(char * string, char arg)
{ //display the string in stdout "as is"
    int col = 0;
    //go character by character to avoid bugs
    //while not null char '\0' or End-of-FILE
    if (arg == 'p')
        while (string[col] && string[col] != EOF)
            putchar(string[col++]);
    if (arg == 'u')
        while (string[col] && string[col] != EOF)
            putchar(toupper(string[col++]));        
    if (arg == 'l')
        while (string[col] && string[col] != EOF)
            putchar(tolower(string[col++]));
    putchar('\n'); //formatted padding
}
void pfile(char * fname, char arg)
{ //print the files contents to stdout "as is"
    FILE * fp;
    char ch;
    fp = fopen(fname, "r"); //open file for reading
    if(fp == NULL) //attempt failed
    {
        printf("Oops! Something went wrong while attempting to open the file.\n");
        puts("The file may be missing, corrupted, or may not exist.");
        exit(1); //quit program
    }
    //getc(fp) gets a character from the open file
    if (arg == 'p')
        while((ch = getc(fp)) != EOF)
            putchar(ch);
    if (arg == 'u')
        while((ch = getc(fp)) != EOF)
            putchar(toupper(ch));
    if (arg == 'l')
        while((ch = getc(fp)) != EOF)
            putchar(tolower(ch));
    fclose(fp); //close the file
}
int fext(char * string)
{//determine if there is a file extension
    //flag which value to return
    int i, ch, scan, place, flag = false;
    //while string is not a null character,
    //assign flag a boolean value,
    //break while if a character matches
    for (i = 0; string[i]; i++)
    {
        if (string[i] == '.')
        {
            //check if next char is a space char
            if (isspace(string[i + 1])) break;
            //count characters after '.'
            for (scan = i, place = 0; string[i+scan]; scan++) ++place;
            scan = i + 1; //advance by one character
            //only ASCII or Plain Text files are allowed
            if (!strncmp(string+scan,"txt",place))
            {
                flag = true;
                break;
            }
            if (!strncmp(string+scan,"asc",place))
            {
                flag = true;
                break;
            }
            if (!strncmp(string+scan,"c",place))
            {
                flag = true;
                break;
            }
            if (!strncmp(string+scan,"csv",place))
            {
                flag = true;
                break;
            }
            if (!strncmp(string+scan,"html",place))
            {
                flag = true;
                break;
            }
            if (!strncmp(string+scan,"log",place))
            {
                flag = true;
                break;
            }
            if (!strncmp(string+scan,"xhtml",place))
            {
                flag = true;
                break;
            }
            if (!strncmp(string+scan,"xml",place))
            {
                flag = true;
                break;
            }
        }
    }
    return flag; //assume *string is a string literal
}
int option(char * string)
{//determine if string is a formal argument
    auto int i, place, arg = false;
    if (string[0] == '-' && string[1] == '-' && isalpha(string[2]))
    {
        for (i = 0; string[i]; i++) //count alpha based chars
            if (isalpha(string[i])) 
                ++place;
        if (!strncmp((string + 2),"help",place))
        {
            arg = 'h'; //return the help argument
        }
        if (!strncmp((string + 2),"print",place))
        {
            arg = 'p'; //return the argument to print as is
        }
        if (!strncmp((string + 2),"uppercase",place))
        {
            arg = 'u'; //return the uppercase argument
        }
        if (!strncmp((string + 2),"lowercase",place))
        {
            arg = 'l'; //return the lowercase argument
        }
    }
    else if (string[0] == '-' && isalpha(string[1]))
    {
        switch (string[1])
        {
            case 'h':
                arg = 'h';
                break;
            case 'p':
                arg = 'p';
                break;
            case 'u':
                arg = 'u';
                break;
            case 'l':
                arg = 'l';
                break;
            default:
                arg = false;
                break;
        }
    }
    else 
    {
        arg = false;
    }
    return arg; //assume *string is a string literal
}
void help(char * string)
{
    printf("Usage: %s [OPTION]... [FILE] or [STRING]...\n", string);
    puts("Echoes FILE or STRING until End-of-FILE to the display.\n");
    printf("%s%50s\n", "-h, --help", "Prints this help text to the display.");
    printf("%s%49s\n", "-p, --print", "Prints the FILE as is to the display.");
    printf("%s%40s\n", "-u, --uppercase", "Maps the input to all uppercase.");
    printf("%s%40s\n", "-l, --lowercase", "Maps the input to all lowercase.");
}
int main(int argc, char *argv[])
{ //main program
    //extension, option
    auto int ext, opt; 
    switch(argc)
    {
        case 2:
            //check to see if input is an argument
            if ((opt = option(argv[1])) == 'h') //if argument is for help
            {
                help(argv[0]); //print only the help text to the display
                break;
            }
            if (opt) //if only an optional argument is made
            {
                printf("%s: missing FILE or STRING operand\n", argv[0]);
                printf("Try '%s --help' for more information.\n", argv[0]);
                break;
            }
            //check to see if input is string or file
            if (!opt && (ext = fext(argv[1]))) //if a file was provided with no opt
                pfile(argv[1], 'p'); //print FILE as is -- default argument
            else if (!opt && !ext)  //if no opt or file was provided
                display(argv[1], 'p'); //print STRING as is -- default argument
            break;
        case 3:
            if ((opt = option(argv[1])) == 'h')
            {
                help(argv[0]);
                break;
            }
            else if (opt)
                ext = fext(argv[2]);
            else
            {
                opt = false;
                ext = false;
            }
            if (opt && ext) //if option and file extension are valid and present
                pfile(argv[2], opt);
            else if (opt && !ext) //if option is present, but file is missing
                display(argv[2], opt);
            else
            {
                printf("%s: invalid OPTION was provided\n", argv[0]);
                printf("Try '%s --help' for more information.\n", argv[0]);
            }
            break;
        default:
            printf("%s: invalid arguments were made\n", argv[0]);
            printf("Try '%s --help' for more information.\n", argv[0]);
            break;
    }

    return 0;
}

Answer 1

Having a program that sometimes acts as echo and sometimes acts as cat (or type, if you use Windows) depending on the presence of a known file extension in the string argument is, frankly, weird. I don't think that it fulfills the requirements of the stated exercise or has a good use case, but I'll play along.

I agree with @Brythan that the code could benefit greatly from having a blank line or two between functions.

The outline of the code is quite good — I like the way you split up the work into this set of functions. There is a lot that can be simplified within each function, though. I'll start reviewing from the top.

display()

You should generally avoid doing I/O one character at a time, as each call to putchar() can have a relatively large overhead for a system call. A single call to puts() would be better.

The code is essentially repeated three times, with variants for passthrough, uppercase, and lowercase. A good way to avoid such repetition is to specify a transformation, which is passed in as a function pointer. First, we define a passthrough() function that is analogous to toupper() and tolower():

int passthrough(int c) {
    return c;
}

Then, the display() function could be written like this:

void display(char *string, int (*transform)(int))
{
    for (char *s = string; *s; s++)
    {
        *s = transform(*s);
    }
    puts(string);
}

See the discussion of main() below to see how display() is called.

pfile()

print_file() would be a clearer name.

As with display(), I recommend using a transformation function and avoiding putchar().

The error handling could be better. Errors should be printed to stderr, to avoid contaminating stdout. Instead of "something went wrong", you could tell the user exactly what failed. (By the way file corruption won't cause fopen() to fail. Filesystem corruption could cause it to fail. Other failure modes are also possible, such as lack of permission.) I suggest returning -1 on error, and having the caller use perror() to print the diagnostics. Having exit() calls sprinkled in various places around your code makes the function less reusable and your program harder to maintain.

Comments such as exit(1); //quit program and fclose(fp); //close the file are pointless. I recommend omitting them. Put more effort into documenting the function's purpose, parameters, and return values instead.

/**
 * Prints the contents of the file, after applying a transformation, to stdout.
 * Returns the number of bytes printed, if successful, or -1 on error.
 */
int print_file(const char *filename, int (*transform)(int))
{
    size_t size, total = 0;
    char buffer[8192];
    FILE *fp;
    if (NULL == (fp = fopen(filename, "r")))
    {
        return -1;
    }
    while ((size = fread(buffer, 1, sizeof(buffer), fp)))
    {
        total += size;
        for (size_t i = 0; i < size; i++)
        {
            buffer[i] = transform(buffer[i]);
        }
        fwrite(buffer, 1, size, stdout);
    }
    int retval = feof(fp) ? total : -1;
    fclose(fp);
    return retval;
}

fext()

The function should have a better name. It could also be written in a much simpler way.

int known_file_extension(const char *filename)
{
    char *dotext;
    if ((dotext = strrchr(filename, '.')))
    {
        return 0 == strcmp(dotext, ".txt") ||
               0 == strcmp(dotext, ".asc") ||
               0 == strcmp(dotext, ".c") ||
               0 == strcmp(dotext, ".csv") ||
               0 == strcmp(dotext, ".html") ||
               0 == strcmp(dotext, ".log") ||
               0 == strcmp(dotext, ".xhtml") ||
               0 == strcmp(dotext, ".xml");
    }
    return 0;
}

option()

For proper treatment of command-line options, getopt() is the way to go. However, I can understand that getopt() is a bit complicated, and your own code could be easier to follow.

I prefer to think of option() as returning a char, possibly the '-' character if the option is not recognized, or the NUL character if it doesn't look like an option at all.

Here's a simpler implementation.

/**
 * Returns 'h', 'p', 'u', or 'l' if the string contains the help, print,
 * upperase, or lowercase option, respectively.  Returns '-' if the string
 * contains an unrecognized option.  Returns '\0' if string is not an
 * option that starts with a hyphen.
 */
char option(const char *string)
{
    if (string[0] == '-' && string[1] == '-')
    {
        if (0 == strcmp(string + 2, "help"))      return 'h';
        if (0 == strcmp(string + 2, "print"))     return 'p';
        if (0 == strcmp(string + 2, "uppercase")) return 'u';
        if (0 == strcmp(string + 2, "lowercase")) return 'l';
        return '-';
    }
    else if (string[0] == '-' && string[1] != '\0' && string[2] == '\0')
    {
        switch (string[1])
        {
          case 'h':
          case 'p':
          case 'u':
          case 'l':
            return string[1];
        }
        return '-';
    }
    return '\0';
}

help()

Generally, help functions need to print to stdout if the help was explicitly requested, or to stderr if the program was called with unreasonable parameters.

I suggest printing just the basename of the program (stripping out the directory, if any).

For formatting, don't hard-code weird column widths while right-aligning the second column. Instead, specify a fixed width on the first column.

In the text, "until End-of-File" is confusingly placed after "STRING".

void help(FILE *out, char *argv0)
{
    fprintf(out, "Usage: %s [OPTION]... [STRING] or [FILE]...\n"
                 "Echoes STRING or FILE until End-of-FILE to the display.\n\n",
                 basename(argv0));
    fprintf(out, "%-23s%s\n", "-h, --help",
                              "Prints this help text to the display.");
    fprintf(out, "%-23s%s\n", "-p, --print",
                              "Prints the FILE as is to the display.");
    fprintf(out, "%-23s%s\n", "-u, --uppercase",
                              "Maps the input to all uppercase.");
    fprintf(out, "%-23s%s\n", "-l, --lowercase",
                              "Maps the input to all lowercase.");
}

main()

The logic would be simpler if you didn't treat the two-argument and three-argument cases separately.

If the parameters are invalid, you might as well show the help message instead of telling the user to run the program with the --help option.

int main(int argc, char *argv[])
{
    int (*transform)(int) = passthrough;    // passthrough, toupper, or tolower
    char *string = NULL;

    for (int i = 1; i < argc; i++)
    {
        switch (option(argv[i]))
        {
          case '-':
            help(stderr, argv[0]);
            return 1;
          case 'h':
            help(stdout, argv[0]);
            return 0;
          case 'p':
            transform = passthrough;
            break;
          case 'u':
            transform = toupper;
            break;
          case 'l':
            transform = tolower;
            break;
          default:
            string = argv[i];
        }
    }
    if (!string)
    {
        fprintf(stderr, "%s: missing FILE or STRING operand\n", basename(argv[0]));
        help(stderr, argv[0]);
        return 1;
    }

    if (known_file_extension(string))
    {
        if (-1 == print_file(string, transform))
        {
            perror(string);
            return 2;
        }
    }
    else
    {
        display(string, transform);
    }
}

Answer 2

*/
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <ctype.h>
#define TITLE "Chapter 11 Exercise 13"
_Bool true = 1, false = 0;
void display(char * string, char arg)

I would find this easier to read with more vertical spacing.

*/

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

#define TITLE "Chapter 11 Exercise 13"

_Bool true = 1, false = 0;

void display(char * string, char arg)

Now I can quickly see where the comment section ends and the include section begins. Then there's a define section; a definition; and finally the code begins.

if(fp == NULL) //attempt failed
{

I would avoid putting comments between a control statement and its statement or block. Actually, I avoid putting comments to the right of code in general. I'd prefer

//attempt failed
if ( NULL == fp )
{

or

if ( NULL == fp )
{
    //attempt failed

I also flipped the equality check. Done this way, NULL = fp will throw a compiler error. The other way, that typo will sort of silently do the wrong thing and lead to a run-time error later in the code.

    exit(1); //quit program

Try to avoid comments that just say what the code does. Your code should generally be readable enough that comments that explain what are unnecessary. The time to use comments is to explain why.

Also, I'd have written this as

    exit(EXIT_FAILURE);

This is clearer about why you are returning that value. You are already including stdlib.h, so you have EXIT_FAILURE and EXIT_SUCCESS available.

for (i = 0; string[i]; i++)
{
    if (string[i] == '.')
    {

You can do this if you want. I personally would prefer

while ( NULL != string )
{
    if ( '.' == *(string++) )
    {

That would leave the string pointing after the period.

Even better:

char *extension;
extension = strrchr( string, '.' );
if ( NULL == extension ) {
    return false;
}

That creates a pointer to the string after the period.

        if (isspace(string[i + 1])) break;

This seems unnecessary. Spaces are valid characters in filenames. Seldom used and problematic but valid. There's nothing in your original program that would be thrown off by a space.

        for (scan = i, place = 0; string[i+scan]; scan++) ++place;

Then I'd replace that line with

    place = strlen(extension);

After that, do

switch ( place ) 
{
    case 6:
        if ( 0 == strcmp(extension, "xhtml")
        {
            return true;
        }
        break;

Add the other extension lengths as necessary. Note that there is no point in using strncmp here as your original code relies on the string being null terminated. If strcmp were problematic, you'd have already hit that problem. Your original code would also have matched file.xht with the xhtml extension which seems unlikely to be what you want.

I also removed your flag variable. It's unnecessary since you can just return directly. Some people prefer to have only one return at the end of a statement, but if you're going to do that, you should generally avoid other flow control statements, like break. So you'd change your later if statements to else if and add a ! flag check to your outer loop. E.g.

for (i = 0; string[i] && ! flag; i++)

But all this is unnecessary. You check the file extension and use the result to determine if the file is a text file. This can go wrong either way. The file README is almost always a text file, but your program would refuse to read it. If I save an image or other binary with a .txt extension, your program will accept it. There's a Unix command called file which analyzes the contents of a file. You could use a system call to get the results of that or presumably access a library somewhere that does the same thing.

If you use a system call, be careful to quote it properly. This is where a space (or worse, a semicolon) could be problematic. Test with at least "test.txt'; echo 'Oops' '" as an argument.

auto int i, place, arg = false;

You never have to use the auto keyword in C. All variables are declared auto by default (unless explicitly declared static or extern). If you have some weird corner case where this matters, you should comment here.

if (string[0] == '-' && string[1] == '-' && isalpha(string[2]))
{
    for (i = 0; string[i]; i++) //count alpha based chars
        if (isalpha(string[i])) 
            ++place;
    if (!strncmp((string + 2),"help",place))

Again, I think place is unnecessary. Note that if there is one or more non-alpha characters at the end of the string, that this will match if the rest matches. Unless you want that behavior, you can skip the place calculation. Also, it's unclear why you keep going if you encounter a non-alpha character. You could just return at that point, since the string will never match.

You might want to look into how Getopt works. That could help save you writing your own scaffolding in cases like this.

        if (!opt && (ext = fext(argv[1]))) //if a file was provided with no opt

Rather than checking the extension, why not check if the file exists? If it does, then you can print the file if it is text based. If not, then you can print the entry as a string.

return 0;

You don't need to return 0 at the end of main. If you let it fall through, the compiler will add this for you.