Request critique of first program

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.

/**
* 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);

/*
* 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);

/**
* 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) {

/**
* 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;
}

/*
* 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);

/**
* 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;
}

Advertisements

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>

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.

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 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.

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.

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.

Share This Page

Welcome to The Coding Forums!

Welcome to the Coding Forums, the place to chat about anything related to programming and coding languages.

Please join our friendly community by clicking the button below - it only takes a few seconds and is totally free. You'll be able to ask questions about coding or chat with the community and help others.
Sign up now!