c function pointers

Question

This program sorts lines alphabetically/numerically depending on the arguments passed to main.

And im working on this exercise from k&R now: Add the option -f to fold upper and lower case together, so that case distinctions are not made during sorting; for example, a and A compare equal.

Is what i wrote in my_strcmp good? And will it work good in conjuction with -r and -n? (r - reverse order, n- numerical sort).

I figured id ask here for your opinions since on klc wiki the solution isnt presented like this.

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

#define MAXBUF 10000

#define MAXLINES 5000
char *lineptr[MAXLINES];

int readlines(char *lineptr[], char buffer[], int nlines);
void writelines(char *lineptr[], int nlines);

void qsort(void **lineptr, int left, int right, int (*comp)(void *, void *));
int numcmp(char *, char *);

int reverse = 0;
int numeric = 0;
int fold = 0;
int my_strcmp(char *s1, char *s2)
{

     char *p1 = (reverse) ? s2 : s1;
     char *p2 = (reverse) ? s1 : s2;


     if(fold) {
         while(toupper(*p1) == toupper(*p2)) {
            p1++, p2++;
            if(!*p1)
              return 0;
         }
         return toupper(*p1) - toupper(*p2);
     }

     return strcmp(p1, p2);
}

int main(int argc, char *argv[])
{
    int nlines;
    char buffer[MAXBUF];

    int i = 1;
    for(i = 1; i < argc; i++) {
          if(strcmp(argv[i], "-n") == 0) {
             numeric = 1;
          } else if(strcmp(argv[i], "-r") == 0) {
                 reverse = 1;
          } else if(strcmp(argv[i], "-f") == 0) {
                 fold = 1;
          } else {
                 printf("illegal argument\n");
          }
    }


    if((nlines = readlines(lineptr, buffer, MAXLINES)) >= 0) {
        qsort((void **)lineptr, 0, nlines - 1, 
        (numeric ? (int (*)(void *, void *))numcmp : (int (*)(void *, void *))my_strcmp));
        writelines(lineptr, nlines);
        getchar();
        return 0;
    } else {
           printf("input too big to sort\n");
           return 1;
    }

}

void writelines(char *lineptr[], int nlines)
{
    int i;

    for (i = 0; i < nlines; i++)
        printf("%s\n", lineptr[i]);
}

int getline(char s[], int lim)
{
    int c, i;

    for (i=0; i<lim-1 && (c=getchar())!=EOF && c!='\n'; ++i)
        s[i] = c;
    if (c == '\n') {
        s[i] = c;
        ++i;
    }
    s[i] = '\0';
    return i;
}

#define MAXLEN 1000
int readlines(char *lineptr[], char *buffer, int maxlines)
{
    int len, nlines;
    char *p, line[MAXLEN];

    nlines = 0;
    p = buffer;
    while ((len = getline(line, MAXLEN)) > 0)
        if (p - buffer + len > MAXBUF)
            return -1;
        else {
            line[len-1] = '\0'; /* delete newline */
            strcpy(p, line);
            lineptr[nlines++] = p;
            p += len;
        }
    return nlines;
}

void qsort(void *v[], int left, int right, int(*comp)(void *, void *))
{
     int i, last;
     void swap(void *v[], int, int);

     if(left >= right)
        return;

     swap(v, left, (left + right) / 2);
     last = left;
     for(i = left + 1; i <= right; i++) 
        if((*comp)(v[i], v[left]) < 0)
           swap(v, ++last, i);
     swap(v, left, last);
     qsort(v, left, last - 1, comp);
     qsort(v, last + 1, right, comp);
}

#include <stdlib.h>

int numcmp(char *p1, char *p2)
{
  char *s1 = reverse ? p2 : p1;
  char *s2 = reverse ? p1 : p2;
  double v1, v2;

  v1 = atof(s1);
  v2 = atof(s2);
  if (v1 < v2)
    return -1;
  else if (v1 > v2)                                     
    return 1;      
  else
    return 0;
}

void swap(void *v[], int i, int j)
{
     void *temp;

     temp = v[i];
     v[i] = v[j];
     v[j] = temp;
}

One more question. I was trying to add the option -d (directory order) - "which makes comparisons only on letters numbers and blanks. Make sure it works in conjuction with -f." and im a bit baffled on how to edit my_strcmp. This is what i did:

int my_strcmp(char *s1, char *s2)
{

     char *p1 = (reverse) ? s2 : s1;
     char *p2 = (reverse) ? s1 : s2;

     if(directory) {
         while(!isdigit(*p1) && !isspace(*p1) && !isalpha(*p1) && *p1)
           p1++;

         while(!isdigit(*p2) && !isspace(*p2) && !isalpha(*p2) && *p2)
           p2++;
     }

     if(fold) {
         while(toupper(*p1) == toupper(*p2)) {
            p1++, p2++;
            if(!*p1)
              return 0;
            if(directory) {
               while(!isdigit(*p1) && !isspace(*p1) && !isalpha(*p1))
                   p1++;

               while(!isdigit(*p2) && !isspace(*p2) && !isalpha(*p2))
                   p2++;         
            }
         }
         return toupper(*p1) - toupper(*p2);
     }

     return my_strcmp2(p1, p2);
}

But im not so sure if its good.

I also wrote my_strcmp2, which handles the case if directory and fold are zero.

Then we just strcmp them, but we have to track if directory is 1 aswell...

int my_strcmp2(char *s1, char *s2)
{
    char *p1 = (reverse) ? s2 : s1;
    char *p2 = (reverse) ? s1 : s2;

     while(*p1 == *p1 && *p1) {
        p1++;
        p2++;

         if(directory) {
             while(!isdigit(*p1) && !isspace(*p1) && !isalpha(*p1) && *p1)
               p1++;

             while(!isdigit(*p2) && !isspace(*p2) && !isalpha(*p2) && *p2)
               p2++;
         }
     }
     return *p1 - *p2;
}

Answer 1

When fold==1, my_strcmp will return 0 when p1 is a prefix of p2. You can fix this by moving the line if(!*p1) return 0 to the beginning of the while loop. Other than that it looks good.

Edit: Regarding your second question: You are on the right track with incrementing p1 and p2 for ignored characters. However, this function won't work in non-folding mode (it seems to call itself infinitely). (this was fixed by an edit to the question)

I would make a helper function compareCharactes that would compare 2 characters with or without case sensitivity, depending on the value of fold. Then you could use your while loop whether fold is on or off.

Edit2: OK, you keep changing your question... Anyway, if you take my advice on the compareCharacters function, there would be no need for the separate my_strcmp and my_strcmp2 functions. You could just write while (compareCharacters(*p1, *p2)==0) {.....}

Answer 2

You'll have to set your own standards of success. Write down test cases, sets of two strings and the output they should produce. Verify them by running the code. Don't forget the outliers, pass empty strings and NULL strings too.

Answer 3

It looks like my_strcmp should work, but I think I'd do things just a bit differently. You really have three possibilities: case senstive, case-insensitive, and numeric comparison. I'd do something like:

typedef int (*cmp_func)(void *, void *);
cmp_func funcs[] = {strcmp, my_strcmp, numcmp};

enum comparison_types = { case_sensitive, case_insensitive, numeric};

int comparison = case_sensitive;

// ...

if (strcmp(argv[i], "-f"))
    comparison = case_insensitive;
else if (strcmp(argv[i], "-n"))
    comparison = numeric;

// ...
   qsort(/* ... */, funcs[comparison]);

Using this, my_strcmp would only to case-insensitive comparisons. Adding directory-order comparisons would involve adding another separate function, instead of expanding (further) on my_strcmp, which is already pretty big, and already does two different things (and with directory comparison, would do three, or six including reverse comparisons).

Answer 4

at first glance it looks OK to me.

Does it compile and do the right thing, a compiler and OS is a much better code validator than I am.