5
\$\begingroup\$

I wrote a sorting program so that if the optional argument -i is present then the sorting should be done without taking into account lowercase / uppercase letters.

The program uses the standard library qsort() algorithm; case-insensitive option is handled by getopt() if the -i flag is supplied to the program.

Please criticize my implementation.

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

/* simple comparison function
   just plain string compare from standard library
   qsort passes to this a pointer to a member of the to be sorted array
   this member is a string, so a cast is needed to double pointer
*/
int simple_cmp(const void *a, const void *b)
{
    return strcmp(*(const char **)a, *(const char **)b);
}

int insensitive_cmp(const void *a, const void *b)
{
    return strcasecmp(*(const char **)a, *(const char **)b);
}

/* natural comparison function
   contrary to the other required sorting methods, standard library does not
   have a function for this (actually there is one for directory contents,
   but it is a specialized function not dedicated to string comparison)
   it acts as a normal comparison as long as there is no digits to compare
   when both strings have digits as the next character, instead of comparing
   them, turn it into a number (all the consecutive digits) and compare that
   number as if it was a single character
*/
int natural_cmp(const void *a, const void *b)
{
    const char *stra = *(const char **)a;
    const char *strb = *(const char **)b;

    while (*stra && *strb)
    {
        if (((*stra < '0') || (*stra > '9')) ||
            ((*strb < '0') || (*strb > '9')))
        {
            if (*stra < *strb)
                return -1;
            else if (*stra > *strb)
                return 1;
            stra++;
            strb++;
        }
        else
        {
            long long na;
            long long nb;
            char *end;
            na = strtoll(stra, &end, 10);
            stra = end;
            nb = strtoll(strb, &end, 10);
            strb = end;
            if (na < nb)
                return -1;
            if (na > nb)
                return 1;
        }
    }
    if (*stra != 0)
        return 1;
    if (*strb != 0)
        return -1;
    return 0;
}


int main(int argc,char *argv[])
{
    int i, iFlag = 0, nFlag = 0, rFlag = 0, nrFlaguri = 0;
    int opt;
    int (*sort_func)(const void *, const void *) = simple_cmp;

    /* getopt will cycle through the parameters searching for optional arguments
       if the specified argument is found store the required configuration
       one cannot parse normal arguments until getopt finished cycling through
       all the arguments
       getopt will rearrange the parameter list, so all positional arguments
       are moved to the end, hence normal processing can be done only after
       all optional arguments are consumed
       if unknonw options are detected exit the program with an error message
       as it is most likely a user error or typo and should be corrected
       to achieve expected results
    */
    while ((opt = getopt(argc, argv, "nri")) != -1)
    {
        switch (opt)
        {
            case 'n':
        nrFlaguri++;
        nFlag = 1;    
                sort_func = natural_cmp;
                break;
        case 'r':
        nrFlaguri++;
        rFlag = 1;
        break;
        case 'i':
        nrFlaguri++;
        iFlag = 1;
        sort_func = insensitive_cmp;
        break;
            default: /* '?' */
                exit(EXIT_FAILURE);
        }
    }
    
    if(argc == optind)
    {
    printf("Only optional parameters have been entered");
    return 0;
    }

    if(nrFlaguri > 1)
    {
    printf("Only one sorting option can be selected");
    return 0;
    }

    /* after rearranging the arguments, optind is updated to reflect the
       starting position of the first non-optional argument
       all parameter processing after getopt finished should start from optind
       instead of 1
    */

    /* quick-sort as implemented in the standard library
       it sorts in-place a given vector of unspecified type elements
       the size of a single element, and the length of vector must be supplied
       along the vector itself
       additionally a comparison function must be specified which can compare
       individual elements of the vector
    */
    qsort(&argv[optind], argc-optind, sizeof(char *), sort_func);
    if (rFlag == 1) {
    for(i=argc-1;i>=optind;i--)
        printf("%s\n",argv[i]);
    }
    else {
    /* displaying sorted parameters one by one */
    for(i=optind;i<argc;i++)
        printf("%s\n",argv[i]);
    }
    return 0;
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Nice work delegating simple_cmp and insensitive_cmp in a way that is compliant. \$\endgroup\$
    – Neil
    Commented May 10, 2022 at 4:17

2 Answers 2

4
\$\begingroup\$

Bug: String handling is unsigned

For all functions in this subclause, each character shall be interpreted as if it had the type unsigned char (and therefore every possible object representation is valid and has a different value). C17dr § 7.24.1.3

Below compares incorrectly*1*2 when *stra and *strb signs differ.

const char *stra = *(const char **)a;
const char *strb = *(const char **)b;
   ...
        if (*stra < *strb)

Instead as below.

const unsigned char *stra = *(const unsigned char **)a;
const unsigned char *strb = *(const unsigned char **)b;

Other code may also need changing.

Note that if (*stra != 0) return 1; assumes *stra > 0.

Why long long?

With na = strtoll(stra, &end, 10);, rather than use long long, consider intmax_t for the longest well supported signed integer type. Today, this might be the same, but plan for tomorrow.

Define when needed

        // long long na;
        // ...
        // na = strtoll(stra, &end, 10);

        long long na = strtoll(stra, &end, 10);

Sign-less?

if (((*stra < '0') || (*stra > '9')) || ((*strb < '0') || (*strb > '9'))) prevents integer compare should the number begin with a sign '+' or '-' or white-space.

I'd either allow integers to begin with a sign or use unsigned long long na = strtoull(stra, &end, 10) for additional range.

Further: If staying with positive base ten, consider comparing digits as text, then there is no real upper integer size bound.

Only base 10?

Consider strtoll(stra, &end, 0); instead of strtoll(stra, &end, 10); to allow input like 0x1234.

Not a standard C solution

Neither #include <unistd.h>, strcasecmp() nor getopt() are part of the standard C library. If coding to a specific implementation, best to tag the question as such.

Use an auto-formatter

Below betrays manual formatting. Increase productivity and uniformity - use an auto-formatter.

    switch (opt)
    {
        case 'n':
    nrFlaguri++;
    nFlag = 1;    
            sort_func = natural_cmp;
            break;
    case 'r':
    nrFlaguri++;
    rFlag = 1;
    break;

*1 Compliant strcmp() behaves as if the characters were unsigned, even if char is signed.

*2 There also exist an issue is a signed char is using non-2's complement that is solved correctly with unsigned char *. But those encodings are rare these days.

\$\endgroup\$
2
  • \$\begingroup\$ The explicit casts are unnecessary when converting from (perhaps cv-qualified) pointers-to-void, so we could have const unsigned char *const *pa = a; const unsigned char *stra = *pa; (ignoring the fact that stra is a reserved identifier). \$\endgroup\$ Commented Aug 23 at 9:57
  • \$\begingroup\$ @TobySpeight True. \$\endgroup\$
    – chux
    Commented Aug 23 at 15:34
1
\$\begingroup\$

Sorting is not locale-aware

Consider using strxfrm() to convert inputs so that the comparisons respect user's LC_COLLATE setting. Obviously this will require storage to be allocated (with care, we can make a single call to malloc() sufficient space for all the transformed strings).

Avoid reserved identifiers

The C standard says that all names beginning with str followed by a letter are reserved for future use by <string.h>. So don't name a variable stra or strb (str_a and str_b are acceptable alternatives).

Unused variables

nFlag and iFlag are both initialised to 0, but never accessed after that. This suggests you haven't enabled enough compiler warnings.

Distinguish errors from normal operation

Don't write error messages to stdout - stderr exists for that purpose. And remember to terminate each line.

Also, don't exit with a success status if there's an error.

        fputs("Only optional parameters have been entered\n", stderr);
        return EXIT_FAILURE;
        fprintf(stderr, "%d sorting options selected, but only one allowed\n", nrFlaguri);
        return EXIT_FAILURE;
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.