Request critique of first program

Discussion in 'C Programming' started by cs, Sep 2, 2007.

  1. cs

    cs Guest

    Hi,

    I'm new to C and would appreciate any feedback on the following
    program, asplit, which splits a file into 2 new files, putting a
    certain number of lines in the first file, and all the rest in the
    second file.

    Any comments as to non-portability, stylistic infelicities, outright
    bugs or anything else would be very much appreciated.

    It compiles fine using gcc 4.1.2, and --std=c99 -pedantic -Wall, with
    no warnings, and seems to work.

    There are two files, a header and a source file.

    ## Begin asplit.h ##

    #ifndef ASPLIT_H_
    #define ASPLIT_H_

    enum e_status { SUCCESS, ERROR_FILE_INPUT, ERROR_FILE_OUTPUT_A,
    ERROR_FILE_OUTPUT_B };

    typedef enum e_status status;

    typedef struct {
    int status;
    long int num_rem_lines;
    } asplit_result;

    /**
    * Asymmetrically splits file named in_file into 2 files, out_file_a,
    and
    * out_file_b, putting num_lines (must be non-negative) lines in the
    first
    * file, and the rest in the second file.
    *
    * Any of the filenames may be "-", indicating stdin in the case of
    in_file,
    * or stdout in the case of out_file_a or out_file_b.
    *
    * Returns a pointer to an asplit_result struct (unless unable to
    allocate,
    * in which case it returns NULL), which shows success or failure and
    the
    * number of lines that were included in 2nd file if successful.
    * Possible status values are given by the e_status enumeration in
    this
    * header file. Success is indicated by SUCCESS, and each of the 3
    different
    * failure states is indicated by one of the enum values beginning
    with
    * ERROR_, indicating which file was unable to be opened.
    *
    * If the status is SUCCESS, then the num_rem_lines value indicates
    * the number of lines remaining after the first file was filled,
    * all of which were entered into the second file.
    *
    * The client is responsible for memory management of the result
    struct.
    *
    */
    asplit_result * asplit(const char *in_file, const char *out_file_a,
    const char *out_file_b,
    const long int num_lines);

    #endif /*ASPLIT_H_*/

    ## End asplit.h ##

    ## Begin asplit.c ##

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

    #include <string.h>

    #include <getopt.h>

    #include "asplit.h"

    char *copy_string(const char *const s);

    int close_file(FILE * f);

    FILE * open_infile(const char * fname);

    FILE * open_outfile(const char *fname);

    /* Name under which program was invoked. */
    char *program_name;

    void usage(int status) {

    if (status != EXIT_SUCCESS)
    fprintf(stderr, "Try `%s -h' for more information.\n",
    program_name);
    else {
    printf(
    "\n\
    Usage: %s [OPTION] -i IN_FILE -a OUT_FILE_A -b OUT_FILE_B -n NUM_LINES
    \n\n\
    ",
    program_name);
    puts("\
    Asymmetrically split IN_FILE into 2 files, putting the first NUM_LINES
    \n\
    into OUT_FILE_A and the rest into OUT_FILE_B.\n\
    \n\
    -i input file, IN_FILE\n\
    -a name of first output file, OUT_FILE_A\n\
    -b name of second output file, OUT_FILE_B\n\
    -n the number of lines to put in OUT_FILE_A\n\
    \n\
    -h display this help and exit\n\
    -v verbose output\n\
    ");
    puts("\
    \n\
    When IN_FILE is -, use standard input; when OUT_FILE_A or OUTFILE_B is
    -,\n\
    use standard output.\n\
    ");
    printf(
    "\
    \n\
    Examples:\n\n\
    %s -n 100 -i - -a output1 -b output2\n\
    Read from standard input, writing first 100 lines to output1
    file and\n\
    all following lines to output2.\n\n\
    %s -n 1000 -i infile.txt -a output1.txt -b -\n\
    Read from infile.txt, writing the first 1000 lines to
    output1.txt and\n\
    all following lines to standard output.\n\n\
    %s -n 1 -i - -a - -b -\n\
    Copy standard input to standard output.\n\n\
    \n",
    program_name, program_name,
    program_name);
    puts("\nReturns 0 on success, and 1 on failure.\n");
    printf("\nReport bugs to .
    \n\n");
    }
    exit(status);
    }

    asplit_result * asplit(const char *in_file, const char *out_file_a,
    const char *out_file_b,
    const long int num_lines) {

    long int curr_line_num;
    int curr_chr;
    FILE *in_f, *out_f;
    asplit_result *result;

    size_t size = sizeof(asplit_result);
    result = malloc(size);
    if (result == NULL) {
    return NULL;
    } else {
    result->num_rem_lines = 0;
    }

    /* Open in_file if not "-", returning unsuccessfully if unable
    to open. */
    if ((in_f = open_infile(in_file)) == NULL) {
    result->status = ERROR_FILE_INPUT;
    return result;
    }

    /* If num_lines is 0, then nothing goes into the first file.*/
    if (num_lines == 0) {
    /*
    * So open second file if not "-", returning
    unsuccessfully
    * if unable to open.
    */
    if ((out_f = open_outfile(out_file_b)) == NULL) {
    close_file(in_f);
    result->status = ERROR_FILE_OUTPUT_B;
    return result;
    }
    } else {
    /*
    * If num_lines is greater than 0, then open first
    file, returning
    * unsuccessfully if unable to open.
    */
    if ((out_f = open_outfile(out_file_a)) == NULL) {
    close_file(in_f);
    result->status = ERROR_FILE_OUTPUT_A;
    return result;
    }
    }

    /* Initial line number is 0, and increments after each newline
    is read. */
    curr_line_num = 0;

    /* While input file still has characters: */
    while ((curr_chr = getc(in_f)) != EOF) {

    /* Output char to current output file. */
    putc(curr_chr, out_f);

    /*
    * If we read a newline, and if we've just hit the
    maximum
    * allowed in the first file, we must close the first
    file, and
    * open the second file, returning unsuccessfully if
    unable to open.
    */
    if (curr_chr == '\n'&& ++curr_line_num == num_lines) {
    close_file(out_f);
    if ((out_f = open_outfile(out_file_b)) ==
    NULL) {
    close_file(in_f);
    result->status = ERROR_FILE_OUTPUT_B;
    return result;
    }
    }
    }
    /* Close input and output files if necessary. */
    close_file(in_f);
    close_file(out_f);

    /* Return the number of lines output to the second file. */
    result->status = 0;
    result->num_rem_lines = (curr_line_num < num_lines) ? 0 :
    (curr_line_num - num_lines);
    return result;

    }

    int main(int argc, char **argv) {

    program_name = *argv;

    char *input_file = NULL;
    char *output_file_a = NULL;
    char *output_file_b = NULL;

    int num_lines = -1;
    int verbose = 0;
    asplit_result *result = NULL;

    int ich;

    while ((ich = getopt(argc, argv, "hvi:a:b:n:")) != EOF) {
    switch (ich) {
    case 'h':
    usage(0);
    break;
    case 'i':
    input_file = (char *) copy_string(optarg);
    break;
    case 'a':
    output_file_a = (char *) copy_string(optarg);
    break;
    case 'b':
    output_file_b = (char *) copy_string(optarg);
    break;
    case 'n':
    num_lines = atoi(optarg);
    if (num_lines < 0)
    printf("-n arg must be at least 0, not
    %d.\n", num_lines);
    break;
    case 'v':
    verbose = 1;
    break;
    default:
    usage(1);
    break;
    }
    }

    /* Verify that all required arguments were submitted. */
    if (input_file == NULL|| output_file_a == NULL|| output_file_b
    == NULL
    || num_lines < 0)
    usage(1);


    /* Do the actual split. */
    result = asplit(input_file, output_file_a, output_file_b,
    num_lines);

    if (result == NULL) {
    fprintf(stderr, "Error: asplit returned NULL result.");
    exit(1);
    }

    /* Determine whether it terminated successfully, and handle
    accordingly. */
    switch (result->status) {
    case SUCCESS:
    if (verbose)
    printf("Output %li lines to 2nd file.\n",
    result->num_rem_lines);
    exit(0);
    case ERROR_FILE_INPUT:
    fprintf(stderr, "Unable to open input file: %s\n",
    input_file);
    exit(1);
    case ERROR_FILE_OUTPUT_A:
    fprintf(stderr, "Unable to open output file: %s\n",
    output_file_a);
    exit(1);
    case ERROR_FILE_OUTPUT_B:
    fprintf(stderr, "Unable to open output file: %s\n",
    output_file_b);
    exit(1);
    default:
    fprintf(stderr, "Error: unexpected return value %d\n", result-
    >status);

    exit(1);
    }

    }

    /**
    * Create a copy of the string referenced by the given pointer,
    returning
    * a pointer to the newly created copy, or NULL if unable to malloc
    memory
    * for the copy or original pointer was null or the string was empty.
    *
    * The client is responsible for memory management of the new string.
    */
    char *copy_string(const char *str_orig) {

    char *str_copy = NULL;

    if (str_orig != NULL && *str_orig != 0) {
    size_t size = strlen(str_orig) + 1;

    str_copy = malloc(size);

    if (str_copy != NULL)
    memcpy(str_copy, str_orig, size);
    }

    return str_copy;

    }

    /**
    * Close the file referenced by the given file pointer if it is not
    one of
    * the standard system files.
    *
    * Returns 0 if the file was actually closed, and 1 if not closed
    */
    int close_file(FILE * f) {
    if (f != stdin&& f != stdout&& f != stderr) {
    fclose(f);
    return 0;
    }
    return 1;
    }

    /**
    * Open the input file (mode 'r') referenced by the given pointer,
    * returning a pointer to the FILE. If fname is "-", return stdin.
    */
    FILE * open_infile(const char *const fname) {
    return strcmp(fname, "-") == 0 ? stdin : fopen(fname, "r");
    }

    /**
    * Open the output file (mode 'w') referenced by the given pointer,
    * returning a pointer to the FILE. If fname is "-", return stdout.
    */
    FILE * open_outfile(const char *const fname) {
    return strcmp(fname, "-") == 0 ? stdout : fopen(fname, "w");
    }

    ## End asplit.c ##

    I did browse the FAQ already and found a couple of things to improve,
    which I already did, but any additional feedback would be appreciated.

    Cheers,

    calvin
     
    cs, Sep 2, 2007
    #1
    1. Advertising

  2. cs

    cs Guest

    Here is an improved version of the source file, based on Peter's
    suggestions.

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

    #include <string.h>

    #include "asplit.h"

    char *copy_string(const char *const s);

    int close_file(FILE * f);

    FILE * open_infile(const char *fname);

    FILE * open_outfile(const char *fname);

    /* Name under which program was invoked. */
    const char *program_name;

    void usage(FILE *fp) {

    fprintf(fp, "\n"
    "Usage: %s [OPTION] -i IN_FILE -a OUT_FILE_A -b OUT_FILE_B -n NUM_LINES
    \n\n"
    , program_name);
    fprintf(fp,
    "Asymmetrically split IN_FILE into 2 files, putting the first NUM_LINES
    \n"
    "into OUT_FILE_A and the rest into OUT_FILE_B.\n\n"
    " -i input file, IN_FILE\n"
    " -a name of first output file, OUT_FILE_A\n"
    " -b name of second output file, OUT_FILE_B\n"
    " -n the number of lines to put in OUT_FILE_A\n
    \n"
    " -h display this help and exit\n"
    " -v verbose output\n");
    fprintf(fp, "\n"
    "When IN_FILE is -, use standard input; when OUT_FILE_A or OUTFILE_B
    is -,\n"
    "use standard output.\n");
    fprintf(fp, "\n"
    "Examples:\n\n"
    " %s -n 100 -i - -a output1 -b output2\n"
    " Read from standard input, writing first 100 lines to output1
    file\n"
    " and all following lines to output2.\n\n"
    " %s -n 1000 -i infile.txt -a output1.txt -b -\n"
    " Read from infile.txt, writing the first 1000 lines to
    output1.txt\n"
    " and all following lines to standard output.\n\n"
    " %s -n 1 -i - -a - -b -\n"
    " Copy standard input to standard output.\n\n"
    , program_name, program_name, program_name);
    fprintf(fp, "\nReturns 0 on success, and 1 on failure.\n");
    fprintf(fp, "\nReport bugs to .\n
    \n");
    exit(fp == stderr ? EXIT_FAILURE : EXIT_SUCCESS);

    }

    int asplit(const char *in_file, const char *out_file_a,
    const char *out_file_b, const long int num_lines,
    long int *num_rem_lines) {

    long int curr_line_num;
    int curr_chr;
    FILE *in_f, *out_f;

    /* Open in_file if not "-", returning unsuccessfully if unable to
    open. */
    if ((in_f = open_infile(in_file)) == NULL) {
    return ASPLIT_INPUT_ERROR;
    }

    /* If num_lines is 0, then nothing goes into the first file.*/
    if (num_lines == 0) {
    /*
    * So open second file if not "-", returning unsuccessfully
    * if unable to open.
    */
    if ((out_f = open_outfile(out_file_b)) == NULL) {
    close_file(in_f);
    return ASPLIT_OUTPUT_B_ERROR;
    }
    } else {
    /*
    * If num_lines is greater than 0, then open first file,
    returning
    * unsuccessfully if unable to open.
    */
    if ((out_f = open_outfile(out_file_a)) == NULL) {
    close_file(in_f);
    return ASPLIT_OUTPUT_B_ERROR;
    }
    }

    /* Initial line number is 0, and increments after each newline
    char. */
    curr_line_num = 0;

    /* While input file still has characters: */
    while ((curr_chr = getc(in_f)) != EOF) {

    /* Output char to current output file. */
    putc(curr_chr, out_f);

    /*
    * If we read a newline, and if we've just hit the maximum
    * allowed in the first file, we must close the first file,
    and
    * open the second file, returning unsuccessfully if unable to
    open.
    */
    if (curr_chr == '\n'&& ++curr_line_num == num_lines) {
    close_file(out_f);
    if ((out_f = open_outfile(out_file_b)) == NULL) {
    close_file(in_f);
    return ASPLIT_OUTPUT_B_ERROR;
    }
    }
    }
    /* Close input and output files if necessary. */
    close_file(in_f);
    close_file(out_f);

    /* Set num_rem_lines, and return success. */
    if (curr_line_num < num_lines)
    *num_rem_lines = 0;
    else
    *num_rem_lines = curr_line_num - num_lines;

    return ASPLIT_SUCCESS;

    }

    int main(int argc, char **argv) {

    program_name = *argv;

    if (argc == 1)
    usage(stdout);

    char *input_file = NULL;
    char *output_file_a = NULL;
    char *output_file_b = NULL;

    int num_lines = -1;
    int verbose = 0;

    for(int i = 1; i < argc; ++i) {
    if (strcmp(argv, "-h") == 0) {
    usage(stdout);
    } else if (strcmp(argv, "-v") == 0) {
    verbose = 1;
    } else if (strcmp(argv, "-i") == 0) {
    if (argc == i+1)
    usage(stderr);
    input_file = argv[++i];
    } else if (strcmp(argv, "-a") == 0) {
    if (argc == i+1)
    usage(stderr);
    output_file_a = argv[++i];
    } else if (strcmp(argv, "-b") == 0) {
    if (argc == i+1)
    usage(stderr);
    output_file_b = argv[++i];
    } else if (strcmp(argv, "-n") == 0) {
    if (argc == i+1)
    usage(stderr);
    char *end_ptr;
    num_lines = strtol(argv[++i], &end_ptr, 10);
    if (*end_ptr != '\0' || num_lines < 0) {
    fprintf(stderr, "-n arg must be a non-negative integer.
    \n");
    usage(stderr);
    }
    } else {
    /* None of the expected arguments. */
    usage(stderr);
    }
    }

    /* Verify that all required arguments were submitted. */
    if (input_file == NULL || output_file_a == NULL || output_file_b
    == NULL
    || num_lines < 0)
    usage(stderr);


    /* Do the actual split. */
    long int num_rem_lines = 0;
    status ret_status = asplit(input_file, output_file_a,
    output_file_b, num_lines, &num_rem_lines);

    /* Determine whether it terminated successfully, and handle
    accordingly.*/
    switch (ret_status) {
    case ASPLIT_SUCCESS:
    if (verbose)
    printf("Output %li lines to 2nd file.\n", num_rem_lines);
    exit(EXIT_SUCCESS);
    case ASPLIT_INPUT_ERROR:
    fprintf(stderr, "Unable to open input file: %s\n",
    input_file);
    exit(EXIT_FAILURE);
    case ASPLIT_OUTPUT_A_ERROR:
    fprintf(stderr, "Unable to open output file: %s\n",
    output_file_a);
    exit(EXIT_FAILURE);
    case ASPLIT_OUTPUT_B_ERROR:
    fprintf(stderr, "Unable to open output file: %s\n",
    output_file_b);
    exit(EXIT_FAILURE);
    default:
    fprintf(stderr, "Error: unexpected return value: %d\n",
    ret_status);
    exit(EXIT_FAILURE);
    }

    }

    /**
    * Close the file referenced by the given file pointer if it is not
    one of
    * the standard system files.
    *
    * Returns 0 on success, and non-zero if unable to close file.
    */
    int close_file(FILE * f) {
    if (f != stdin&& f != stdout&& f != stderr) {
    return fclose(f);
    }
    return 0;
    }

    /**
    * Open the input file (mode 'r') referenced by the given pointer,
    * returning a pointer to the FILE. If fname is "-", return stdin.
    */
    FILE * open_infile(const char *const fname) {
    return strcmp(fname, "-") == 0 ? stdin : fopen(fname, "r");
    }

    /**
    * Open the output file (mode 'w') referenced by the given pointer,
    * returning a pointer to the FILE. If fname is "-", return stdout.
    */
    FILE * open_outfile(const char *const fname) {
    return strcmp(fname, "-") == 0 ? stdout : fopen(fname, "w");
    }
     
    cs, Sep 2, 2007
    #2
    1. Advertising

  3. On Sun, 02 Sep 2007 03:24:15 -0000, cs <> wrote:

    >Hi,
    >
    >I'm new to C and would appreciate any feedback on the following
    >program, asplit, which splits a file into 2 new files, putting a
    >certain number of lines in the first file, and all the rest in the
    >second file.
    >
    >Any comments as to non-portability, stylistic infelicities, outright
    >bugs or anything else would be very much appreciated.
    >
    >It compiles fine using gcc 4.1.2, and --std=c99 -pedantic -Wall, with
    >no warnings, and seems to work.
    >
    >There are two files, a header and a source file.
    >
    >## Begin asplit.h ##
    >
    >#ifndef ASPLIT_H_
    >#define ASPLIT_H_
    >
    >enum e_status { SUCCESS, ERROR_FILE_INPUT, ERROR_FILE_OUTPUT_A,
    > ERROR_FILE_OUTPUT_B };
    >
    >typedef enum e_status status;
    >
    >typedef struct {
    > int status;


    While member names and typdef types occupy different name spaces,
    using the same name for two different purposes will only confuse
    people who read your code, including yourself later on.

    > long int num_rem_lines;
    >} asplit_result;
    >
    >/**
    > * Asymmetrically splits file named in_file into 2 files, out_file_a,
    >and
    > * out_file_b, putting num_lines (must be non-negative) lines in the
    >first
    > * file, and the rest in the second file.
    > *
    > * Any of the filenames may be "-", indicating stdin in the case of
    >in_file,
    > * or stdout in the case of out_file_a or out_file_b.
    > *
    > * Returns a pointer to an asplit_result struct (unless unable to
    >allocate,
    > * in which case it returns NULL), which shows success or failure and
    >the
    > * number of lines that were included in 2nd file if successful.
    > * Possible status values are given by the e_status enumeration in
    >this
    > * header file. Success is indicated by SUCCESS, and each of the 3
    >different
    > * failure states is indicated by one of the enum values beginning
    >with
    > * ERROR_, indicating which file was unable to be opened.
    > *
    > * If the status is SUCCESS, then the num_rem_lines value indicates
    > * the number of lines remaining after the first file was filled,
    > * all of which were entered into the second file.
    > *
    > * The client is responsible for memory management of the result
    >struct.
    > *
    > */
    >asplit_result * asplit(const char *in_file, const char *out_file_a,
    >const char *out_file_b,
    > const long int num_lines);
    >
    >#endif /*ASPLIT_H_*/
    >
    >## End asplit.h ##
    >
    >## Begin asplit.c ##
    >
    >#include <stdio.h>
    >#include <stdlib.h>
    >
    >#include <string.h>
    >
    >#include <getopt.h>


    A non-standard header which many of us don't have.

    >
    >#include "asplit.h"
    >
    >char *copy_string(const char *const s);
    >
    >int close_file(FILE * f);
    >
    >FILE * open_infile(const char * fname);
    >


    Unnecessary double spacing makes it very difficult to see enough of
    your program in one screen view.

    >FILE * open_outfile(const char *fname);
    >
    >/* Name under which program was invoked. */
    >char *program_name;
    >
    >void usage(int status) {
    >
    > if (status != EXIT_SUCCESS)
    > fprintf(stderr, "Try `%s -h' for more information.\n",
    >program_name);
    > else {
    > printf(
    > "\n\
    >Usage: %s [OPTION] -i IN_FILE -a OUT_FILE_A -b OUT_FILE_B -n NUM_LINES
    >\n\n\
    >",


    Make life easy on yourself. C will automatically merge adjacent
    string literals together for you. When you have a very long string
    literal, break it into smaller pieces like
    "\n"
    "Usage: %s [OPTION] -i IN_FILE -a OUT_FILE_A"
    " -b OUT_FILE_B -n NUM_LINES\n\n"

    This eliminates the need for the escape character \ at the end of each
    line and solves the problem of Usenet wrapping your text.

    snip rest of function

    >}
    >
    >asplit_result * asplit(const char *in_file, const char *out_file_a,
    >const char *out_file_b,
    > const long int num_lines) {
    >
    > long int curr_line_num;
    > int curr_chr;
    > FILE *in_f, *out_f;
    > asplit_result *result;
    >
    > size_t size = sizeof(asplit_result);
    > result = malloc(size);


    Most of us use
    result = malloc(sizeof *result);
    especially since you don't reuse size in this function.

    > if (result == NULL) {
    > return NULL;
    > } else {
    > result->num_rem_lines = 0;
    > }
    >
    > /* Open in_file if not "-", returning unsuccessfully if unable
    >to open. */
    > if ((in_f = open_infile(in_file)) == NULL) {
    > result->status = ERROR_FILE_INPUT;
    > return result;
    > }
    >
    > /* If num_lines is 0, then nothing goes into the first file.*/
    > if (num_lines == 0) {
    > /*
    > * So open second file if not "-", returning
    >unsuccessfully
    > * if unable to open.
    > */
    > if ((out_f = open_outfile(out_file_b)) == NULL) {
    > close_file(in_f);
    > result->status = ERROR_FILE_OUTPUT_B;
    > return result;
    > }
    > } else {
    > /*
    > * If num_lines is greater than 0, then open first
    >file, returning
    > * unsuccessfully if unable to open.
    > */
    > if ((out_f = open_outfile(out_file_a)) == NULL) {
    > close_file(in_f);


    Aren't you going to close out_file_b also?

    > result->status = ERROR_FILE_OUTPUT_A;
    > return result;
    > }
    > }
    >
    > /* Initial line number is 0, and increments after each newline
    >is read. */
    > curr_line_num = 0;
    >
    > /* While input file still has characters: */
    > while ((curr_chr = getc(in_f)) != EOF) {
    >
    > /* Output char to current output file. */
    > putc(curr_chr, out_f);
    >
    > /*
    > * If we read a newline, and if we've just hit the
    >maximum
    > * allowed in the first file, we must close the first
    >file, and
    > * open the second file, returning unsuccessfully if
    >unable to open.
    > */
    > if (curr_chr == '\n'&& ++curr_line_num == num_lines) {
    > close_file(out_f);
    > if ((out_f = open_outfile(out_file_b)) ==
    >NULL) {
    > close_file(in_f);
    > result->status = ERROR_FILE_OUTPUT_B;
    > return result;
    > }
    > }
    > }
    > /* Close input and output files if necessary. */
    > close_file(in_f);
    > close_file(out_f);
    >
    > /* Return the number of lines output to the second file. */
    > result->status = 0;


    You defined SUCCESS in your enum. Why don't you use it?

    > result->num_rem_lines = (curr_line_num < num_lines) ? 0 :
    >(curr_line_num - num_lines);
    > return result;
    >
    >}
    >
    >int main(int argc, char **argv) {
    >
    > program_name = *argv;
    >
    > char *input_file = NULL;
    > char *output_file_a = NULL;
    > char *output_file_b = NULL;
    >
    > int num_lines = -1;
    > int verbose = 0;
    > asplit_result *result = NULL;
    >
    > int ich;
    >
    > while ((ich = getopt(argc, argv, "hvi:a:b:n:")) != EOF) {


    Non-standard function many of us won't have and, for those that do, it
    may not behave the same as yours.

    > switch (ich) {
    > case 'h':
    > usage(0);


    usage tests for EXIT_SUCCESS, not for 0. Be consistent. The two are
    equivalent when returning from main() but need not be equal.

    > break;
    > case 'i':
    > input_file = (char *) copy_string(optarg);


    copy_string already returns a char*. What purpose does the cast
    serve?

    > break;
    > case 'a':
    > output_file_a = (char *) copy_string(optarg);
    > break;
    > case 'b':
    > output_file_b = (char *) copy_string(optarg);
    > break;
    > case 'n':
    > num_lines = atoi(optarg);


    atoi doesn't provide for any post-return error checking. strtol is
    better. And you should add the error checking.

    > if (num_lines < 0)
    > printf("-n arg must be at least 0, not
    >%d.\n", num_lines);
    > break;
    > case 'v':
    > verbose = 1;
    > break;
    > default:
    > usage(1);
    > break;
    > }
    > }
    >
    > /* Verify that all required arguments were submitted. */
    > if (input_file == NULL|| output_file_a == NULL|| output_file_b
    >== NULL
    > || num_lines < 0)
    > usage(1);


    At this point, you know you have an error. Do you really want to go
    through the rest of the code with erroneous user input?

    >
    >
    > /* Do the actual split. */
    > result = asplit(input_file, output_file_a, output_file_b,
    >num_lines);
    >
    > if (result == NULL) {
    > fprintf(stderr, "Error: asplit returned NULL result.");
    > exit(1);
    > }
    >
    > /* Determine whether it terminated successfully, and handle
    >accordingly. */
    > switch (result->status) {
    > case SUCCESS:
    > if (verbose)
    > printf("Output %li lines to 2nd file.\n",
    >result->num_rem_lines);
    > exit(0);
    > case ERROR_FILE_INPUT:
    > fprintf(stderr, "Unable to open input file: %s\n",
    >input_file);
    > exit(1);
    > case ERROR_FILE_OUTPUT_A:
    > fprintf(stderr, "Unable to open output file: %s\n",
    >output_file_a);
    > exit(1);
    > case ERROR_FILE_OUTPUT_B:
    > fprintf(stderr, "Unable to open output file: %s\n",
    >output_file_b);
    > exit(1);
    > default:
    > fprintf(stderr, "Error: unexpected return value %d\n", result-
    >>status);

    > exit(1);
    > }
    >
    >}
    >
    >/**
    > * Create a copy of the string referenced by the given pointer,
    >returning
    > * a pointer to the newly created copy, or NULL if unable to malloc
    >memory
    > * for the copy or original pointer was null or the string was empty.
    > *
    > * The client is responsible for memory management of the new string.
    > */
    >char *copy_string(const char *str_orig) {
    >
    > char *str_copy = NULL;
    >
    > if (str_orig != NULL && *str_orig != 0) {
    > size_t size = strlen(str_orig) + 1;
    >
    > str_copy = malloc(size);
    >
    > if (str_copy != NULL)
    > memcpy(str_copy, str_orig, size);
    > }
    >
    > return str_copy;
    >
    >}
    >
    >/**
    > * Close the file referenced by the given file pointer if it is not
    >one of
    > * the standard system files.
    > *
    > * Returns 0 if the file was actually closed, and 1 if not closed
    > */
    >int close_file(FILE * f) {
    > if (f != stdin&& f != stdout&& f != stderr) {
    > fclose(f);
    > return 0;
    > }
    > return 1;
    >}
    >
    >/**
    > * Open the input file (mode 'r') referenced by the given pointer,
    > * returning a pointer to the FILE. If fname is "-", return stdin.
    > */
    >FILE * open_infile(const char *const fname) {
    > return strcmp(fname, "-") == 0 ? stdin : fopen(fname, "r");
    >}
    >
    >/**
    > * Open the output file (mode 'w') referenced by the given pointer,
    > * returning a pointer to the FILE. If fname is "-", return stdout.
    > */
    >FILE * open_outfile(const char *const fname) {
    > return strcmp(fname, "-") == 0 ? stdout : fopen(fname, "w");
    >}
    >
    >## End asplit.c ##
    >
    >I did browse the FAQ already and found a couple of things to improve,
    >which I already did, but any additional feedback would be appreciated.



    Remove del for email
     
    Barry Schwarz, Sep 3, 2007
    #3
  4. cs

    cs Guest

    On Sep 2, 6:47 pm, Barry Schwarz <> wrote:
    > On Sun, 02 Sep 2007 03:24:15 -0000, cs <> wrote:
    >
    > While member names and typdef types occupy different name spaces,
    > using the same name for two different purposes will only confuse
    > people who read your code, including yourself later on.


    Thanks, I agree that it's slightly confusing with no redeeming
    benefit, and had changed it since posting the original version, in the
    second version that I posted [1].

    > A non-standard header which many of us don't have.


    I since took out the dependency on getopt, based on earlier advice
    from Peter Nilsson.

    > >#include "asplit.h"

    >
    > >char *copy_string(const char *const s);

    >
    > >int close_file(FILE * f);

    >
    > >FILE * open_infile(const char * fname);

    >
    > Unnecessary double spacing makes it very difficult to see enough of
    > your program in one screen view.


    I see. I thought it seemed strange to have function prototypes without
    an extra newline between them, but if that's not considered bad style,
    I'll not doublespace the prototypes anymore. The more on one screen,
    the better (other things being equal).

    >
    > Make life easy on yourself. C will automatically merge adjacent
    > string literals together for you.
    > <snip/>
    >
    > This eliminates the need for the escape character \ at the end of each
    > line and solves the problem of Usenet wrapping your text.


    Thanks, I had updated that in my latest version [1] based on Peter
    Nilsson's advice.

    > > size_t size = sizeof(asplit_result);
    > > result = malloc(size);

    >
    > Most of us use
    > result = malloc(sizeof *result);
    > especially since you don't reuse size in this function.


    Thanks. A small but definite improvement. My updated version does away
    with the struct altogether.

    >
    > > /* Open in_file if not "-", returning unsuccessfully if unable
    > >to open. */
    > > if ((in_f = open_infile(in_file)) == NULL) {
    > > result->status = ERROR_FILE_INPUT;
    > > return result;
    > > }

    >
    > > /* If num_lines is 0, then nothing goes into the first file.*/
    > > if (num_lines == 0) {
    > > /*
    > > * So open second file if not "-", returning
    > >unsuccessfully
    > > * if unable to open.
    > > */
    > > if ((out_f = open_outfile(out_file_b)) == NULL) {
    > > close_file(in_f);
    > > result->status = ERROR_FILE_OUTPUT_B;
    > > return result;
    > > }
    > > } else {
    > > /*
    > > * If num_lines is greater than 0, then open first
    > >file, returning
    > > * unsuccessfully if unable to open.
    > > */
    > > if ((out_f = open_outfile(out_file_a)) == NULL) {
    > > close_file(in_f);

    >
    > Aren't you going to close out_file_b also?


    The only way that out_file_b can be open at this point is if num_lines
    was 0, in which case the else isn't executed and I will continue
    execution. Further down, out_file_b is closed.

    > > /* Return the number of lines output to the second file. */
    > > result->status = 0;

    >
    > You defined SUCCESS in your enum. Why don't you use it?


    Thanks. That was an oversight on my part. I had meant to use it, and
    it is used in the latest version I posted.

    > > while ((ich = getopt(argc, argv, "hvi:a:b:n:")) != EOF) {

    >
    > Non-standard function many of us won't have and, for those that do, it
    > may not behave the same as yours.


    I realize that now, and rewrote the latest version to not use getopt
    at all. Thanks for the suggestion.

    > > switch (ich) {
    > > case 'h':
    > > usage(0);

    >
    > usage tests for EXIT_SUCCESS, not for 0. Be consistent. The two are
    > equivalent when returning from main() but need not be equal.


    Yes, very sloppy on my part. The latest version takes a FILE pointer
    instead of the exit status--based again on a suggestion of Peter
    Nilsson--and determines whether its a normal call of usage or not by
    whether the FILE pointer is to stderr or not.

    >
    > atoi doesn't provide for any post-return error checking. strtol is
    > better. And you should add the error checking.


    Thanks, I had since discovered strtol, and the latest posted version
    [1] used strtol instead.

    >
    > > if (num_lines < 0)
    > > printf("-n arg must be at least 0, not
    > >%d.\n", num_lines);
    > > break;
    > > case 'v':
    > > verbose = 1;
    > > break;
    > > default:
    > > usage(1);
    > > break;
    > > }
    > > }

    >
    > > /* Verify that all required arguments were submitted. */
    > > if (input_file == NULL|| output_file_a == NULL|| output_file_b
    > >== NULL
    > > || num_lines < 0)
    > > usage(1);

    >
    > At this point, you know you have an error. Do you really want to go
    > through the rest of the code with erroneous user input?
    >


    Yes, much better to call usage and exit immediately after "if
    (num_lines < 0)". My later version had actually already made that
    change.

    Thanks for the many good suggestions. I should have spent a bit more
    time looking over the code before posting. Many of the issues had
    nothing at all to do with being new to C but were just sloppy
    oversights.

    Cheers,

    Calvin

    [1] <http://groups.google.com/group/comp.lang.c/msg/189178d4caf3fc7e?
    dmode=source>
     
    cs, Sep 3, 2007
    #4
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Fao, Sean

    Critique Request: CheckBoxColumn

    Fao, Sean, Feb 15, 2006, in forum: ASP .Net
    Replies:
    0
    Views:
    523
    Fao, Sean
    Feb 15, 2006
  2. cs

    Request critique of first program

    cs, Sep 2, 2007, in forum: C Programming
    Replies:
    14
    Views:
    497
    Keith Thompson
    Sep 4, 2007
  3. Zack

    Critique of first python code

    Zack, Feb 8, 2008, in forum: Python
    Replies:
    9
    Views:
    416
    George Sakkis
    Feb 17, 2008
  4. voipfc
    Replies:
    0
    Views:
    139
    voipfc
    Oct 11, 2006
  5. kazack
    Replies:
    3
    Views:
    290
    Tad McClellan
    Oct 22, 2003
Loading...

Share This Page