please critique this code

charleswoerner
charleswoerner used Ask the Experts™
on
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]);
}
Comment
Watch Question

Do more with

Expert Office
EXPERT OFFICE® is a registered trademark of EXPERTS EXCHANGE®
Top Expert 2005
Commented:
Traditionally NULL refers to a null pointer   (void*)(0)
You are defining NUL is the character '\0'   (char)(0)
But in the code, you are using NULL as the double 0.0  (double)(0).
But it will not match, because your scanf statement is storing a
decimal integer 0 because the format string is incorrect (it should
be "%g" not "%d".

You seem to have a difficulty distinguishing between data
and pointers to data.  For instance:
 int i;           // an integer
 int*ip = &i; // a pointer to an integer;
 double input[MAXNUM];     // an array of double precision floating point numbers
 double *inputs[MAXNUM];  // an array of pointers to double precision floating point numbers


Although you defined input as an array of pointers to doubles,
you are using it as an array of doubles in main(), but as an
array of pointers to doubles in sort().  You will have to make
up your mind.  If you keep it an array of pointers, you will then
need to allocate the memory it points to (scanf doesn't allocate
it for you).

sizeof(buffer) is the size of the buffer in bytes and dividing by
sizeof(char) is dividing by 1.


I'm not quite sure what you are trying to do here. I suppose it
tries to flush input to the end of line.
   fgets(buffer, sizeof(buffer)/sizeof(char), stdin);


The pointer to the ops structure, s_opts, in get_opts() is uninitialized.
You will need to allocate memory accordingly.  In this case, it is
common practice that the calling routine supplies the memory.
In main()
  struct opts s_opts;
...
  get_opts(&s_opts, argc, argv);

In your compare functions, alpha() and reverse(), the tirneries
are redundant  (a > b) evaluates to 1 if a is greater than b and
0 otherwise.  So
 int retval = (a > b) ? 1 : 0;   is the same as
 int retval = (a > b);

And the stack variable retval too is redundant, so accomplished
C programmers would write the one line function body:
  return (a > b);


In C, boolean operations use 0 for FALSE and 1 for TRUE, and your
presorted() routines are actually returning booleans (as ints).  However
the naming "presorted" lends to a bit of confusion: if presorted is TRUE,
you swap the items.  Intuitively, I would think that if presorted is TRUE,
the items were already in order and didn't require swapping.  You might
be better to rename "presorted" to "needsSorting".


Again, in print_numbers(), the printf format string says you are
printing out a decimal integer, but you are supplying a pointer  to
a double.  It would correctly be:
 printf("%g\n", *input[i]);


That is enough for now.  As the compiler says:
 "Too many errors... giving up."
Both main and get_opts function declare a pointer to a opts  struct. However you use this structure without ever allocating memory for this structure. In the main I would declare:

struct opts s_opts; // this will create an instance of the struct allocating memory rather than just a pointer to a struct

Then you should pass in the adress of the structure to the get_opts function which I would delcare like this:
void get_opts(int argc, char *argv[], struct opts *s_opt);

get_opts function must not have this line anymore:
struct opts *s_opt;
Also drop the return statement as the function is now delcared as void and will modidy the struct that you pass in.

In the main use s_opts.is_alpha instead of s_opts->is_alpha as you no longer have to dereference a pointer.

In function get_opts there is no checking for invalid options for example if you pass a paramter of z instead of -z you the default values will never be used. It will be best to setup the default values at the start of the function and then based on the paramters modify them. No default will be required in the case statement. Alternately you can have the function return a error code when an invalid parameter is passed and act on that in the main.

Also why do you have a loop to process the command line paramters when you are only expecting one parameter? Looks like you are doing so because the first parameter is the program executable. In your program you can simple parse the second paramter, and if you every have to loop through a number of paramters skip the first which is the exe name.

Why do you need a struct in the first place to determine the sort order? If all you use is a simple flag to determine sort order.

Also regarding the format strings instead of %d you should use %f or %g when dealing with floating point numbers as pointed out by brettmjohnson, but this is for float. For double you should use %lf of %lg and for int use %d and long %ld

I would like to help some more but this program as so many errors I will leave some for other experts to help out.

Commented:
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?

Author

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

Author

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

Author

Commented:
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

Author

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

Author

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

Do more with

Expert Office
Submit tech questions to Ask the Experts™ at any time to receive solutions, advice, and new ideas from leading industry professionals.

Start 7-Day Free Trial