Return to Answer

• 1

• 7k
• 3
• 22
• 37

• 7k
• 3
• 22
• 37

*/
#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.

{ //display the string in stdout "as is"

You call this C, but that's a C++ comment. This will almost certainly work, as almost all C compilers support C++ (or most people use a C++ compiler to compile C). It doesn't feel quite right though.

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.