2
\$\begingroup\$

I have this following code to sort an array of strings order by ASCII value (alphabetically) using bubble sort.

#include <stdio.h>
#include <string.h>
int main(){
    char txt1[201][10], temp[201];
    for(int i=0; i < 10; i++){
        if(i){scanf("%c", txt1[i]);}
        scanf("%[^\n]s", txt1[i]);
    }

    for(int i=0; i<9; i++){
        for(int j=i+1; j<10; j++){
            if(strcmp(txt1[i], txt1[j]) > 0){
                strcpy(temp, txt1[i]);
                strcpy(txt1[i], txt1[j]);
                strcpy(txt1[j], temp);
            }
        }
    }
    for(int i=0; i < 10; i++){
        printf("%s\n", txt1[i]);
    }
}

Sample

> Input
Bravo
Charlie
Foxtrot
Alpha
Golf
Delta
Echo
Hotel
India
Juliet

> Output
Alpha
Bravo
Charlie
Delta
Echo
Foxtrot
Golf
Hotel
India
Juliet

But I have a concern at line 6-7 (In the first for loop). I need to input value with space so I used %[^\n]s (to receive a string with space) and %c (to receive space between each input).

Will it produce any problem when sorting if using this method on inputting the value?

\$\endgroup\$
2

1 Answer 1

3
\$\begingroup\$

Style

It is more readable to include spaces around binary operators such as the = assignment operator or comparison operators. int i = 0; is better than int i=0;.

You declared temp at the top level of the function even though it is used only inside the loop and does not need to maintain its value between loop iterations. It is better practice to declare variables so that they are only visible in the scopes in which they are used. As such, please declare temp inside the loop instead of at the top.

scanf() loop

The first loop that gets the newline-delimited input can be rewritten without an additional if, as to skip any leading white characters, by adding a leading space to the scanf() format string. You should also add the max length the format string (200) to prevent undefined behavior should a user input a string longer than the allotted size:

for (int i = 0; i < 10; ++i){
    scanf(" %200[^\n]", txt1[i]);
//         ^ This space skips leading newlines
}

Use pointers instead of copying

Your second loop copies strings to other locations during the sorting process. Given that your program is a simple command line tool and presumably does not require all the strings to be sorted in memory, I would suggest maintaining an array of pointers to each string and sorting the pointers based on the strings they point to instead of sorting the strings themselves, but doing this would require declaring a new list for the pointers and initializing the values:

char txt[201][10], *ptr[10];
for (int i = 0; i < 10; ++i) {
    ptr[i] = txt[i]; // This makes the ptr[i] point to the 201-byte buffer at txt[i]
    scanf(" %200[^\n]", txt[i]);
}

You could write the loop body in 1 line instead of 2 if you so desire, though some may advise against combining multiple statements into one like this:

scanf(" %200[^\n]", ptr[i] = txt[i]);

Instead of using strcpy() we would simply update the pointers, which is much more efficient:

for (int i = 0; i < 9; ++i) {
    for (int j = i + 1; j < 10; ++j) {
        if (strcmp(ptr[i], ptr[j]) > 0) {
            char *const temp = ptr[i];
            ptr[i] = ptr[j];
            ptr[j] = temp;
        }
    }
}

Then modify the final loop to print the sorted pointers:

for (int i = 0; i < 10; ++i) {
    puts(ptr[i]); // puts() can be used instead of printf() for a single string followed by a newline
}

Revised Code:

#include <stdio.h>
#include <string.h>
int main(void) {
    char txt[201][10], *ptr[10];
    for (int i = 0; i < 10; ++i) {
        ptr[i] = txt[i];
        scanf(" %200[^\n]", txt[i]);
    }
    for (int i = 0; i < 9; ++i) {
        for (int j = i + 1; j < 10; ++j) {
            if (strcmp(ptr[i], ptr[j]) > 0) {
                char *const temp = ptr[i];
                ptr[i] = ptr[j];
                ptr[j] = temp;
            }
        }
    }
    for (int i = 0; i < 10; ++i) {
        puts(ptr[i]);
    }
    return 0;
}

Additional Revision

Some users in comments have suggested further improvements to the code.

Although the txt array is modified through the scanf() loop, none of the pointers in the ptr list are used to modify the data. As such, they can be declared as pointers to const char.

Additionally, for those who want to avoid Magic Numbers, you should define the maximum size of each string and the string count as macros.

#include <stdio.h>
#include <string.h>
#define MAX_LEN 200
#define NUM_STR 10
#define STR(X) #X
#define STRING(X) STR(X) // https://stackoverflow.com/a/3859980
int main(void) {
    char txt[MAX_LEN + 1][NUM_STR];
    const char *ptr[NUM_STR];

    for (int i = 0; i < NUM_STR; ++i) {
        ptr[i] = txt[i];
        scanf(" %" STRING(MAX_LEN) "[^\n]", txt[i]);
    }
    for (int i = 0; i < NUM_STR - 1; ++i) {
        for (int j = i + 1; j < NUM_STR; ++j) {
            if (strcmp(ptr[i], ptr[j]) > 0) {
                const char *const temp = ptr[i];
                ptr[i] = ptr[j];
                ptr[j] = temp;
            }
        }
    }
    for (int i = 0; i < NUM_STR; ++i) {
        puts(ptr[i]);
    }
    return 0;
}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Consider replacing naked magic numbers 201, 10, 9 with #define LINE_SIZE 200 #define LINES_N 10 or the like. \$\endgroup\$ Mar 17 at 11:38
  • \$\begingroup\$ @chux-ReinstateMonica Do you suggest something like this for the format string? \$\endgroup\$
    – CPlus
    Mar 17 at 16:31
  • \$\begingroup\$ Yes, something like #define LINE_LEN 200 char text[LINE_LEN + 1][LINES_N]; scanf(" %" STRINGIZE(LINE_LEN) "[^\n]", txt[i]); \$\endgroup\$ Mar 18 at 3:36
  • \$\begingroup\$ @chux-ReinstateMonica codereview.stackexchange.com/revisions/291078/2 \$\endgroup\$
    – CPlus
    Mar 18 at 3:56

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.