Name Sorting

[vasanth]

Hi,
I have created a program to sort names... I have used insertion sort here.. and i swap pointers instead of the strings themselves... The program accepts the names through the command line argument.. The program workks but please go through and comment and recomend improvements and i have a feeling i have not handled pointers properly.. it would be grat if you could point them out...

the code is

# include <stdio.h>
# include <stdlib.h>
void insertion (char **a, int n);

int
main (int argc, char *argv[])
{
  int i;

  char **namelist = (char **) calloc (argc - 1, sizeof (char *));

  if (namelist == NULL)
    {
      printf ("\nError allocating memory");
      exit (1);
    }




  for (i = 0; i < argc - 1; i++)
    {
      namelist[i] = (char *) calloc (sizeof (argv[i]), sizeof (char));
      strcpy (namelist[i], argv[i + 1]);
    }

  insertion (namelist, argc - 1);

  for (i = 0; i < argc - 1; i++)
    printf ("%s\n", namelist[i]);



  for (i = 0; i < argc - 1; i++)
    free (namelist[i]);

  free (namelist);


  return 0;
}


void
insertion (char **a, int n)
{

  int i, j;
  char *v;
  for (i = 1; i < n; i++)
    {

      v = a[i];

      j = i;

      while (strcmp (a[j - 1], v) == 1)
	{
	  a[j] = a[j - 1];
	  j = j - 1;
	  if (j <= 0)
	    break;

	}

      a[j] = v;
    }
}

thanx
bye

[Salem]

> namelist[i] = (char *) calloc (sizeof (argv[i]), sizeof (char));
Man, 1200+ posts and you're still mixing sizeof() with strlen()

There's a minor gotcha with using strlen(), so I'm going to see if you can avoid falling into that hole as well.

And you're ignoring the previous suggestions / FAQ entries on not casting malloc.

You're not still using some fossil DOS compiler are you? Because it's only DOS which would tolerate such blatant abuse of memory.


> while (strcmp (a[j - 1], v) == 1)
GO READ THE MANUAL PAGE

DESCRIPTION
       The  strcmp()  function compares the two strings s1 and s2.  It returns
       an integer less than, equal to, or greater than zero if  s1  is  found,
       respectively, to be less than, to match, or be greater than s2.

It's > 0 you want, not == 1

And please include all the correct header files for the functions you use.

> char **namelist = (char **) calloc (argc - 1, sizeof (char *));
I bet you don't know that argc can be zero

> The program workks but please go through and comment
Heh - the sloppy memory allocation and the broken strcmp() call suggests you had some very limited tests.

This is all basic stuff, nevermind the actual implementation of the algorithm in question.

[vasanth]

thanks.. i will go through them and correct them..... i have actually never used malloc but used the new operator(in C++)..I am using gcc but no idea why its not warning me.

[vasanth]

Hi again,
I have modified my code.. can you please review it now and give me suggestions

# include <stdio.h>
# include <stdlib.h>
# include <string.h>

# define MAXLENGTH 20		/*maxlength defined for array to accept names */

void insertion (char *a[], int n);

int
main (void)
{
  int num, i;
  char name[MAXLENGTH];
  char **namelist;

  printf ("\nEnter the number of names you want to enter >");
  scanf ("%d", &num);

  /*An array of pointers is created dynamically and checked for allocation errors */
  if ((namelist =  calloc (num, sizeof (char *))) == NULL)
    {
      printf ("\nError allocating memory");
      exit (1);
    }


  /*
     The loop is run num number of times and names are accepted 
     into the fixed length array name. And memory is dynamically allocated for the length of name
     and the contents are transfered
   */
  for (i = 0; i < num; i++)
    {
      printf ("Enter name %d>", i + 1);
      scanf ("%19s", name);

      if ((*(namelist + i) =calloc (strlen (name), sizeof (char))) == NULL)
	{
	  printf ("\nError allocating memory");
	  for (i--; i >= 0; i--)	/* If an error occurs allocating memory the previous allocations are aslo deallocated and the program quits */
	    free (*(namelist + i));

	  free (namelist);
	  exit (1);
	}
      else
	strcpy (*(namelist + i), name);	/*The name is transfered to the newly allocated space */


    }

  insertion (namelist, num);	/*The sort function is called */


  printf ("\nSORTED ARRAY:\n");

/*
The sorted array is printed and after each print the specific space is deallocated
*/
  for (i = 0; i < num; i++)
    {
      printf ("\n %s", *(namelist + i));
      free (*(namelist + i));
    }

  free (namelist);
  printf ("\n");
  return 0;
}


void
insertion (char *a[], int n)
{
  int i, num;
  char *temphold;		/*Temporary Variable to hold a pointer address when swaping pointers */

  for (i = 1; i < n; i++)	/*Loop is run n number of times which is the number of elements */
    {
      num = i;			/*The value of i is copied into num for manipulation in the while loop */


      /*
         The while loop is run until all the elements in the left section of the array are sorted
       */
      while (strcmp (*(a + num), *(a + num - 1)) < 0)
	{
	  temphold = *(a + num);	/*Swapping part */
	  *(a + num) = *(a + num - 1);
	  *(a + num - 1) = temphold;

	  if (num - 1 == 0)	/*(While) loop terminated on reaching 1st element of the array */
	    break;
	  else
	    num--;		/*Num is decremented to compare previous elements inside a given section */
	}
    }


  return;
}

thanx in advance
bye

[Salem]

> *(namelist + i)
So why did you change this from the more readable namelist[i]?

> calloc (strlen (name), sizeof (char))
LOL - I told you about this trap in advance, and you still fell into it.

[vasanth]

Originally posted by Salem
> *(namelist + i)
So why did you change this from the more readable namelist[i]?

> calloc (strlen (name), sizeof (char))
LOL - I told you about this trap in advance, and you still fell into it.

hmmm i thought that the pointer notation is faster than the subscript notation.. anyways..

could you please explain to me what the trap is..


thanx in advance

[Salem]

> could you please explain to me what the trap is..
The fact that you should malloc strlen(name)+1 bytes if you ever hope to store the \0 at the end of the string successfully.

> hmmm i thought that the pointer notation is faster than the subscript notation
Perhaps you should look at the assembler output and compare.

Perhaps you should understand that [] is just a syntactic variation (which I think is easier to read) and that the compiler treats them both as the same thing.

[vasanth]

ok i analysed what was happening.. if i am right i have to put

> calloc (strlen (name)+1, sizeof (char))

since space needs to be also allocated for the string terminating char... am i right

[vasanth]

Originally posted by vasanth
ok i analysed what was happening.. if i am right i have to put

> calloc (strlen (name)+1, sizeof (char))

since space needs to be also allocated for the string terminating char... am i right

edit
-----



You beat me to it... Thanks .. do you think my deallocation of memory is being done right..

[Salem]

Your deallocation looks good