charleswoerner
asked on
please critique this code
The original question has been replaced by this updated code. I think this is ok now. If you see any glaring weaknesses other than O(n*n) time to sort please submit your response.
#include <stdio.h>
#define MAXNUM 10
#ifndef NULL
#define NULL (double)(0)
#endif
struct opts {
int is_alpha;
int is_reverse ;
};
void get_opts(int argc, char *argv[], struct opts *s_opts);
void sort(double *input[], int n, struct opts *s_opts);
void swap(double **j, double **k);
void print_numbers(double *input[], int n);
int alpha(double j, double i);
int reverse(double i, double j);
void clear_kb(void);
void free_memory(double *input[], int n);
int main(int argc, char *argv[])
{
double *input[MAXNUM];
int n = 0;
struct opts s_opts;
double buffer;
get_opts(argc, argv, &s_opts);
printf("Sort method = %s\n\n", ((s_opts.is_alpha == 1) ? "alpha" : "reverse"));
while (n < MAXNUM)
{
printf("Enter a number>");
scanf("%lf", &buffer);
clear_kb();
if (buffer != NULL)
{
if ((input[n] = (double *)malloc(sizeof(buffer))) != NULL)
{
*input[n++] = buffer;
}
else
{
puts("Memory allocation error.");
exit(1);
}
buffer = NULL;
}
else
puts("That is not a number.");
}
sort(input, n, &s_opts);
print_numbers(input, n);
free_memory(input, n);
return 0;
}
void get_opts(int argc, char *argv[], struct opts *s_opts)
{
int i = 1;
if (argc > 1) {
/* for each argv entry */
for (i; i < argc; i++)
{
/* if it is an option */
if (argv[i][0] == '-')
{
switch (argv[i][1])
{
case 'a':
{
s_opts->is_alpha = 1;
s_opts->is_reverse = 0;
break;
}
case 'z':
{
s_opts->is_alpha = 0;
s_opts->is_reverse = 1;
break;
}
default:
{
s_opts->is_alpha = 1;
s_opts->is_reverse = 0;
}
}
}
}
}
else
{
s_opts->is_alpha = 1;
s_opts->is_reverse = 0;
}
}
void sort(double *input[], int n, struct opts *s_opts)
{
int i, j;
int (*pre_sorted)(double a, double b);
pre_sorted = (s_opts->is_alpha == 1) ? alpha : reverse;
for (i = 1; i <= n; i++)
{
for (j = 0; j < n-1; j++)
{
if (!pre_sorted(*input[j], *input[j+1]))
swap(&input[j], &input[j+1]);
}
}
}
int alpha(double a, double b)
{
return(a < b);
}
int reverse(double a, double b)
{
return(a > b);
}
void swap(double **j, double **k)
{
double *temp;
temp = *j;
*j = *k;
*k = temp;
}
void print_numbers(double *input[], int n)
{
int i;
for (i=0; i<n; i++)
{
printf("%lf\n", *input[i]);
}
}
void clear_kb(void)
{
char junk[256];
fgets(junk, 256, stdin);
}
void free_memory(double *input[], int n)
{
int i;
for (i = 0; i < n; i++)
free(input[i]);
}
#include <stdio.h>
#define MAXNUM 10
#ifndef NULL
#define NULL (double)(0)
#endif
struct opts {
int is_alpha;
int is_reverse ;
};
void get_opts(int argc, char *argv[], struct opts *s_opts);
void sort(double *input[], int n, struct opts *s_opts);
void swap(double **j, double **k);
void print_numbers(double *input[], int n);
int alpha(double j, double i);
int reverse(double i, double j);
void clear_kb(void);
void free_memory(double *input[], int n);
int main(int argc, char *argv[])
{
double *input[MAXNUM];
int n = 0;
struct opts s_opts;
double buffer;
get_opts(argc, argv, &s_opts);
printf("Sort method = %s\n\n", ((s_opts.is_alpha == 1) ? "alpha" : "reverse"));
while (n < MAXNUM)
{
printf("Enter a number>");
scanf("%lf", &buffer);
clear_kb();
if (buffer != NULL)
{
if ((input[n] = (double *)malloc(sizeof(buffer))) != NULL)
{
*input[n++] = buffer;
}
else
{
puts("Memory allocation error.");
exit(1);
}
buffer = NULL;
}
else
puts("That is not a number.");
}
sort(input, n, &s_opts);
print_numbers(input, n);
free_memory(input, n);
return 0;
}
void get_opts(int argc, char *argv[], struct opts *s_opts)
{
int i = 1;
if (argc > 1) {
/* for each argv entry */
for (i; i < argc; i++)
{
/* if it is an option */
if (argv[i][0] == '-')
{
switch (argv[i][1])
{
case 'a':
{
s_opts->is_alpha = 1;
s_opts->is_reverse = 0;
break;
}
case 'z':
{
s_opts->is_alpha = 0;
s_opts->is_reverse = 1;
break;
}
default:
{
s_opts->is_alpha = 1;
s_opts->is_reverse = 0;
}
}
}
}
}
else
{
s_opts->is_alpha = 1;
s_opts->is_reverse = 0;
}
}
void sort(double *input[], int n, struct opts *s_opts)
{
int i, j;
int (*pre_sorted)(double a, double b);
pre_sorted = (s_opts->is_alpha == 1) ? alpha : reverse;
for (i = 1; i <= n; i++)
{
for (j = 0; j < n-1; j++)
{
if (!pre_sorted(*input[j], *input[j+1]))
swap(&input[j], &input[j+1]);
}
}
}
int alpha(double a, double b)
{
return(a < b);
}
int reverse(double a, double b)
{
return(a > b);
}
void swap(double **j, double **k)
{
double *temp;
temp = *j;
*j = *k;
*k = temp;
}
void print_numbers(double *input[], int n)
{
int i;
for (i=0; i<n; i++)
{
printf("%lf\n", *input[i]);
}
}
void clear_kb(void)
{
char junk[256];
fgets(junk, 256, stdin);
}
void free_memory(double *input[], int n)
{
int i;
for (i = 0; i < n; i++)
free(input[i]);
}
ASKER CERTIFIED SOLUTION
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
SOLUTION
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
ASKER
I will award 15 and 10 points so far to brettmjohnson and kirsteng, respectively. I reworked the code and I think it is better, but it doesn't work properly now. The values are in scientific notation and seem to be way off. Here it is... Oh, and I have another 25 points available if you can show my why my output is so *ucked up.
#include <stdio.h>
#define MAXNUM 10
#ifndef NULL
#define NULL (double)(0)
#endif
#define DEBUG 1
struct opts {
int is_alpha;
int is_reverse ;
};
void get_opts(int argc, char *argv[], struct opts *s_opts);
void sort(double *input[], int n, struct opts *s_opts);
void print_numbers(double *input[], int n);
int alpha(double j, double i);
int reverse(double i, double j);
int main(int argc, char *argv[])
{
double *input[MAXNUM];
int n = 0;
struct opts s_opts;
double buffer;
get_opts(argc, argv, &s_opts);
printf("Sort method = %s\n\n", ((s_opts.is_alpha == 1) ? "alpha" : "reverse"));
while (n < MAXNUM)
{
printf("Enter a number>");
fflush(stdin);
scanf("%g", &buffer);
if (buffer != NULL)
{
input[n] = (double *)malloc(sizeof(buffer));
*input[n++] = (double)buffer;
buffer = NULL;
}
else
{
puts("That is not a number.");
fflush(stdin);
}
}
#ifdef DEBUG
int i;
for (i=0; i<n; i++)
{
printf("%g :: %f :: %d\n", input[i], input[i], input[i]);
}
#endif
sort(input, n, &s_opts);
print_numbers(input, n);
return 0;
}
void get_opts(int argc, char *argv[], struct opts *s_opts)
{
int i = 0;
/* for each argv entry */
for (i; i<argc; i++)
{
/* if it is an option */
if (argv[i][0] == '-')
{
switch (argv[i][1])
{
case 'a':
{
s_opts->is_alpha = 1;
s_opts->is_reverse = 0;
break;
}
case 'z':
{
s_opts->is_alpha = 0;
s_opts->is_reverse = 1;
break;
}
default:
{
s_opts->is_alpha = 1;
s_opts->is_reverse = 0;
}
}
}
}
}
void sort(double *input[], int n, struct opts *s_opts)
{
int i, j;
double temp;
int (*pre_sorted)(double a, double b);
pre_sorted = (s_opts->is_alpha == 1) ? alpha : reverse;
for (i = 1; i <= n; i++)
{
for (j = 0; j < n-1; j++)
{
if (!pre_sorted(*input[j], *input[j+1]))
{
temp = *input[j];
*input[j] = *input[j+1];
*input[j+1] = temp;
}
}
}
}
int alpha(double a, double b)
{
return(a < b);
}
int reverse(double a, double b)
{
return(a > b);
}
void print_numbers(double *input[], int n)
{
int i;
for (i=0; i<n; i++)
{
printf("%g\n", *input[i]);
}
}
#include <stdio.h>
#define MAXNUM 10
#ifndef NULL
#define NULL (double)(0)
#endif
#define DEBUG 1
struct opts {
int is_alpha;
int is_reverse ;
};
void get_opts(int argc, char *argv[], struct opts *s_opts);
void sort(double *input[], int n, struct opts *s_opts);
void print_numbers(double *input[], int n);
int alpha(double j, double i);
int reverse(double i, double j);
int main(int argc, char *argv[])
{
double *input[MAXNUM];
int n = 0;
struct opts s_opts;
double buffer;
get_opts(argc, argv, &s_opts);
printf("Sort method = %s\n\n", ((s_opts.is_alpha == 1) ? "alpha" : "reverse"));
while (n < MAXNUM)
{
printf("Enter a number>");
fflush(stdin);
scanf("%g", &buffer);
if (buffer != NULL)
{
input[n] = (double *)malloc(sizeof(buffer));
*input[n++] = (double)buffer;
buffer = NULL;
}
else
{
puts("That is not a number.");
fflush(stdin);
}
}
#ifdef DEBUG
int i;
for (i=0; i<n; i++)
{
printf("%g :: %f :: %d\n", input[i], input[i], input[i]);
}
#endif
sort(input, n, &s_opts);
print_numbers(input, n);
return 0;
}
void get_opts(int argc, char *argv[], struct opts *s_opts)
{
int i = 0;
/* for each argv entry */
for (i; i<argc; i++)
{
/* if it is an option */
if (argv[i][0] == '-')
{
switch (argv[i][1])
{
case 'a':
{
s_opts->is_alpha = 1;
s_opts->is_reverse = 0;
break;
}
case 'z':
{
s_opts->is_alpha = 0;
s_opts->is_reverse = 1;
break;
}
default:
{
s_opts->is_alpha = 1;
s_opts->is_reverse = 0;
}
}
}
}
}
void sort(double *input[], int n, struct opts *s_opts)
{
int i, j;
double temp;
int (*pre_sorted)(double a, double b);
pre_sorted = (s_opts->is_alpha == 1) ? alpha : reverse;
for (i = 1; i <= n; i++)
{
for (j = 0; j < n-1; j++)
{
if (!pre_sorted(*input[j], *input[j+1]))
{
temp = *input[j];
*input[j] = *input[j+1];
*input[j+1] = temp;
}
}
}
}
int alpha(double a, double b)
{
return(a < b);
}
int reverse(double a, double b)
{
return(a > b);
}
void print_numbers(double *input[], int n)
{
int i;
for (i=0; i<n; i++)
{
printf("%g\n", *input[i]);
}
}
ASKER
By the way, this was a practice question in a book I am reading. The assignment was to create a program that accepts 10 numbers from stdin and stores them as an array of pointers to type double. It should then sort the numbers and print out the sorted array. I used a struct for no other reason than to feel comfortable with them.
ASKER
Thank you all so far. I forgot that I had to allocate memory for pointers to structs before assigning their values. It is much easier to just declare a struct (not pointer) then pass the address of the struct. I appreciate the help so far, but can anyone tell me why the #ifdef DEBUG part of the code produces output like...
1.26507e-308 :: 0.000000 :: 1664039526
1.2651e-308 :: 0.000000 :: 1664039526
1.26513e-308 :: 0.000000 :: 1664039526
1.26517e-308 :: 0.000000 :: 1664039526
1.2652e-308 :: 0.000000 :: 1664039526
...and so on
1.26507e-308 :: 0.000000 :: 1664039526
1.2651e-308 :: 0.000000 :: 1664039526
1.26513e-308 :: 0.000000 :: 1664039526
1.26517e-308 :: 0.000000 :: 1664039526
1.2652e-308 :: 0.000000 :: 1664039526
...and so on
ASKER
I gave brettjohnson 30pts and kirsteng 20pts. I will give another 20 if someone can show me what is wrong with the reworked program. Ie. why the numbers that I input (1,4,2,6,5,2.3,6.7,9,2,3) aren't preserved, but rather seem to be converted into something else.
ASKER
I got it all worked out now. Thanks kirsteng. I didn't see the part about the format string using precision modifier l to specify type double pointer. That fixed the output. You get another 20 pts.
#include <stdio.h>
#define MAXNUM 10
#ifndef NULL
#define NULL (double)(0)
#endif
#define DEBUG 1
struct opts {
int is_alpha;
int is_reverse ;
};
void get_opts(int argc, char *argv[], struct opts *s_opts);
void sort(double *input[], int n, struct opts *s_opts);
void print_numbers(double *input[], int n);
int alpha(double j, double i);
int reverse(double i, double j);
int main(int argc, char *argv[])
{
double *input[MAXNUM];
int n = 0;
struct opts s_opts;
double buffer;
get_opts(argc, argv, &s_opts);
printf("Sort method = %s\n\n", ((s_opts.is_alpha == 1) ? "alpha" : "reverse"));
while (n < MAXNUM)
{
printf("Enter a number>");
fflush(stdin);
scanf("%lf", &buffer);
if (buffer != NULL)
{
input[n] = (double *)malloc(sizeof(buffer));
*input[n++] = buffer;
buffer = NULL;
}
else
{
puts("That is not a number.");
fflush(stdin);
}
}
#ifdef DEBUG
int i;
for (i=0; i<n; i++)
{
printf("%lg :: %lf\n", input[i], input[i]);
}
#endif
sort(input, n, &s_opts);
print_numbers(input, n);
return 0;
}
void get_opts(int argc, char *argv[], struct opts *s_opts)
{
int i = 0;
if (argc > 1) {
/* for each argv entry */
for (i; i<argc; i++)
{
/* if it is an option */
if (argv[i][0] == '-')
{
switch (argv[i][1])
{
case 'a':
{
s_opts->is_alpha = 1;
s_opts->is_reverse = 0;
break;
}
case 'z':
{
s_opts->is_alpha = 0;
s_opts->is_reverse = 1;
break;
}
default:
{
s_opts->is_alpha = 1;
s_opts->is_reverse = 0;
}
}
}
}
}
else
{
s_opts->is_alpha = 1;
s_opts->is_reverse = 0;
}
}
void sort(double *input[], int n, struct opts *s_opts)
{
int i, j;
double *temp;
int (*pre_sorted)(double a, double b);
pre_sorted = (s_opts->is_alpha == 1) ? alpha : reverse;
for (i = 1; i <= n; i++)
{
for (j = 0; j < n-1; j++)
{
if (!pre_sorted(*input[j], *input[j+1]))
{
temp = input[j];
input[j] = input[j+1];
input[j+1] = temp;
}
}
}
}
int alpha(double a, double b)
{
return(a < b);
}
int reverse(double a, double b)
{
return(a > b);
}
void print_numbers(double *input[], int n)
{
int i;
for (i=0; i<n; i++)
{
printf("%lf\n", *input[i]);
}
}
#include <stdio.h>
#define MAXNUM 10
#ifndef NULL
#define NULL (double)(0)
#endif
#define DEBUG 1
struct opts {
int is_alpha;
int is_reverse ;
};
void get_opts(int argc, char *argv[], struct opts *s_opts);
void sort(double *input[], int n, struct opts *s_opts);
void print_numbers(double *input[], int n);
int alpha(double j, double i);
int reverse(double i, double j);
int main(int argc, char *argv[])
{
double *input[MAXNUM];
int n = 0;
struct opts s_opts;
double buffer;
get_opts(argc, argv, &s_opts);
printf("Sort method = %s\n\n", ((s_opts.is_alpha == 1) ? "alpha" : "reverse"));
while (n < MAXNUM)
{
printf("Enter a number>");
fflush(stdin);
scanf("%lf", &buffer);
if (buffer != NULL)
{
input[n] = (double *)malloc(sizeof(buffer));
*input[n++] = buffer;
buffer = NULL;
}
else
{
puts("That is not a number.");
fflush(stdin);
}
}
#ifdef DEBUG
int i;
for (i=0; i<n; i++)
{
printf("%lg :: %lf\n", input[i], input[i]);
}
#endif
sort(input, n, &s_opts);
print_numbers(input, n);
return 0;
}
void get_opts(int argc, char *argv[], struct opts *s_opts)
{
int i = 0;
if (argc > 1) {
/* for each argv entry */
for (i; i<argc; i++)
{
/* if it is an option */
if (argv[i][0] == '-')
{
switch (argv[i][1])
{
case 'a':
{
s_opts->is_alpha = 1;
s_opts->is_reverse = 0;
break;
}
case 'z':
{
s_opts->is_alpha = 0;
s_opts->is_reverse = 1;
break;
}
default:
{
s_opts->is_alpha = 1;
s_opts->is_reverse = 0;
}
}
}
}
}
else
{
s_opts->is_alpha = 1;
s_opts->is_reverse = 0;
}
}
void sort(double *input[], int n, struct opts *s_opts)
{
int i, j;
double *temp;
int (*pre_sorted)(double a, double b);
pre_sorted = (s_opts->is_alpha == 1) ? alpha : reverse;
for (i = 1; i <= n; i++)
{
for (j = 0; j < n-1; j++)
{
if (!pre_sorted(*input[j], *input[j+1]))
{
temp = input[j];
input[j] = input[j+1];
input[j+1] = temp;
}
}
}
}
int alpha(double a, double b)
{
return(a < b);
}
int reverse(double a, double b)
{
return(a > b);
}
void print_numbers(double *input[], int n)
{
int i;
for (i=0; i<n; i++)
{
printf("%lf\n", *input[i]);
}
}
but they're used as-is in the compare, so the compare is comparing pointers,
not the double values you're probably expecting.
also the expression (a < b ) ? 1: 0 is redundant- the result of "a < b" is already a C quasi-boolean,
no need to query that result and map it to 1 or 0.
in getopt, you're accessing the string items char s 1 and 2 without
checking to see if they even exist. You may be accessing past the
end of those strings.
The logic of comparing presorted() against zero is a bit
backwards-looking. How about returning TRUE or FALSE instead
if a negatory value?