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

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

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

Problem

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
#include
#include
#include
#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.\

Solution

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':

Code Snippets

int passthrough(int c) {
    return c;
}
void display(char *string, int (*transform)(int))
{
    for (char *s = string; *s; s++)
    {
        *s = transform(*s);
    }
    puts(string);
}
/**
 * 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;
}
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;
}
/**
 * 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';
}

Context

StackExchange Code Review Q#71986, answer score: 10

Revisions (0)

No revisions yet.