6
\$\begingroup\$

I'm trying to brush up on some C basics (it's been ten years since I've programmed in C) and am working on a simple Rot13 program as a Unix filter. Any suggestions on how to improve it would be appreciated. Specifically, I'm curious if the call to fclose at the end of main is necessary or will have bad potential effects since it is possible for stdin to be passed as an argument.

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

// input_from_args: returns a FILE* corresponding to the input
// source specified by the user. If no file was specified,
// returns stdin.

FILE* input_from_args(int argc, const char *argv[])
{
    if (argc == 1) {
        return stdin;
    } else {
        return fopen(argv[1], "r");
    }
}

// rot13: returns the rot13 encoding of the specified character

int rot13(int c)
{
    if (('a' <= c && c <= 'm') || ('A' <= c && c <= 'M')) {
        return c + 13;
    } else if (('n' <= c && c <= 'z') || ('N' <= c && c <= 'Z')) {
        return c - 13;
    } else {
        return c;
    }
}

// convert_all: converts all of the characters in src using the
// rot13 encoding writing the results to dest

void convert_all(FILE *src, FILE *dest)
{
    int c;

    while ((c = fgetc(src)) != EOF) {
        fprintf(dest, "%c", rot13(c));
    }
}

int main(int argc, const char *argv[])
{
    FILE *src = input_from_args(argc, argv);
    FILE *dest = stdout;

    if (src == NULL) {
        fprintf(stderr, "%s: unable to open %s\n", argv[0], argv[1]);
        exit(EXIT_FAILURE);
    }

    convert_all(src, dest);

    fclose(src);

    return EXIT_SUCCESS;
}
\$\endgroup\$
0
\$\begingroup\$

Avoid unusual cases where argc==0

// if (argc == 1) {
if (argc <= 1) {

Consider that the program may be called from a int execv(const char *path, char *const argv[]); where path is as expected yet arg[0] == NULL;.. This meets specification, yet not convention. See can argv[0] have null?

The first argument, by convention, should point to the filename associated with the file being executed. ref

By doing this, code avoids undefined behavior with fopen(argv[1], "r"); and other places.


Minor issues

Add sentinels to bad strings

When a file name is invalid, sometimes its is quite unusual and may include leading/trailing whitespace.

// fprintf(stderr, "%s: unable to open %s\n", argv[0], argv[1]);
fprintf(stderr, "\"%s\": unable to open \"%s\"\n", argv[0], argv[1]);

Missing return?

The below looks wrong as it is missing a return after the if else block

if (argc == 1) {
    return stdin;
} else {
    return fopen(argv[1], "r");
}

Consider instead

if (argc <= 1) {
    return stdin;
}
return fopen(argv[1], "r");

I'm curious if the call to fclose at the end of main is necessary or will have bad potential effects since it is possible for stdin to be passed as an argument.

I am confident that fclose(stdin) right before exit/return is not an issue. Yet given code maintenance and tendency to coders cut/paste good code, I see little to gain by avoiding the simple test

if (src && src != stdin) {
  fclose(src);
}
\$\endgroup\$
1
\$\begingroup\$

While others may have more specific suggestions related to the code, here are my observations:

  • If you're writing this as a Unix filter, make it Unix-y: instead of calling a function rot13, perhaps call it caesar_cipher() and use \$n\$th rotation. This would follow the idea by which Unix systems are characterized: modularity.

  • With modularity: while in the modern age it is safe to assume most systems have contiguous a-z and A-Z character encoding, a solution which accommodates EBCDIC and other locales than C is worth considering.

  • Because you're writing a Unix filter, I would look at the source code for GNU implementations of Unix filters (ie head, tail, uniq) to see how they deal with operations on files and error handling (specifically what types of errors could occur with Unix filters).

  • Ensure you are correctly checking if files exist, permissions, etc. Referencing GNU utils is helpful here as well.

  • Prefer fputc() instead of fprintf(): you're just printing a character to a stream.

Those are my thoughts. Some of these things you may have already considered, but I hope this was helpful.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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