Joseph Martin wrote:
> Well I have successfully completed the first version of my first
> program. I have attached it to this message. If you guys don't mind, could
> you take a look at it, critique it, and point out fallacies? I'd
> appreciate it.
OK. [NB: there was something wrong with the indentation of the
original program; the following version has been re-indented].
#include <stdlib.h>
#include <stdio.h>
#include <strings.h>
#include "fileio.h"
#include "output.h"
int MAXLENGTH = 10;
> int main(int argc, char *argv[])
> {
> /* Declare variables */
> char **index, *home, *file, names[MAXLENGTH][80];
> char (*ptr_to_names)[80], *args;
> float amount[MAXLENGTH];
> int j;
> double m_amount;
>
> /* Initialize all variables */
> file = (char *)malloc(50 * (sizeof(char *)));
`sizeof(char *); should probably be `sizeof(char)', which is
guaranteed to be 1.
> /* Read in budget file */
> home = getenv("HOME");
> sprintf(file, "%s/.budget", home);
Using alloca() would be preferable to malloc() here. Also, you should
compute the correct size for the buffer, e.g.
home = getenv("HOME");
file = alloca(strlen(home) + 9);
sprintf(file, "%s/.budget", home);
> index = read_file(file);
> free(file);
> /* Parse budget file */
> for(j=0; index[j]; j++) {
> sscanf(index[j], "%s %f", &names[j], &amount[j]);
`&names[j]' should just be `names[j]'. An array used as a value is
equivalent to a pointer to the first entry in the array.
> }
> ptr_to_names = names;
This is redundant. You can use names in place of ptr_to_names.
However, I would have created an index array, e.g.
char **ptrs_to_names = malloc(j * sizeof(char *));
int i;
for (i = 0; i < j; i++)
ptrs_to_names[i] = names[i];
That way, the second argument to calculate() could be `char **names',
rather than `char (*names)[80]', which is more flexible. Actually, I
would probably have made `names' a `char **', passed a temporary
buffer to sscanf() and used strdup() to allocate the actual storage
for the strings.
As for the rest of the main program, you should process command line
arguments before doing anything else (e.g. reading ~/.budget). If you
use --help etc, you don't really need to read the file.
Also, you should use getopt() or getopt_long() for argument
processing. That will give you behaviour which is consistent with most
Unix commands, e.g. `-abc' is equivalent to `-a -b -c', the space
between a switch and its argument is optional, arguments following
`--' are never treated as switches, etc.
> /* Main program body */
> // Parse options
> if( argc == 1 ) {
> print_all_info();
> print_help();
> return 0;
> }
> print_version();
> for(j=1; argv[j]; j++)
> {
> if( (strcmp("-h",argv[j]) == 0) || (strcmp("--help",argv[j]) == 0) )
> {
> print_help();
> return 0;
> }
> if( (strcmp("-w",argv[j]) == 0) || (strcmp("--warranty",argv[j]) == 0)
>)
> {
> print_warranty();
> return 0;
> }
> if( (strcmp("-v",argv[j]) == 0) || (strcmp("--version",argv[j]) == 0) )
> {
> return 0;
> }
> if( (strncmp("-m",argv[j],2) == 0) ) {
> args = argv[j];
> args += 2;
> m_amount = strtod(args, '\0');
This should be
m_amount = strtod(args, NULL);
If sizeof(void *) != sizeof(int), it will make a difference.
> if( m_amount != 0 )
> calculate( m_amount, ptr_to_names, amount );
> else {
> puts("Invalid money format.");
> puts("Type budget --help for details");
> return 0;
> }
> }
> }
> return 0;
> }
As for displaying monetary information, you may wish to look up the
details of the LC_MONETARY locale category. This provides support for
internationalisation (aka I18N), so that a program isn't restricted to
a particular set of conventions for displaying monetary amounts (e.g.
US, UK use a dot for the decimal point, whilst most of mainland Europe
uses a comma).
--
Glynn Clements <[EMAIL PROTECTED]>
