patterncModerate
A program that reads input until end-of-file and echoes it to the display
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.\
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
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.
You should generally avoid doing I/O one character at a time, as each call to
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
Then, the
See the discussion of
As with
The error handling could be better. Errors should be printed to
Comments such as
The function should have a better name. It could also be written in a much simpler way.
For proper treatment of command-line options,
I prefer to think of
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':
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.