Link to home
Start Free TrialLog in
Avatar of charleswoerner
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]);
}
ASKER CERTIFIED SOLUTION
Avatar of brettmjohnson
brettmjohnson
Flag of United States of America image

Link to home
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial
SOLUTION
Link to home
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial
Avatar of grg99
grg99

In alpha) and reverse() the arguments are declared as pointers,
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?
Avatar of charleswoerner

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]);
      }
}
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.
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
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.
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]);
      }
}