snippetcMinor
Least significant digit (LSD) radix sort of strings
Viewed 0 times
lsdradixleastsortdigitstringssignificant
Problem
I am learning string algorithms and wrote this C example based on a Java example I found online. Any feedback relating to style, code clarity, whether it is generic enough to be re-used, interface, etc, etc would be very much appreciated.
/*
Demonstration of least significant digit (LSD) radix sort of strings
Specifically UK vehicle registration numbers
*/
#include
#include
#include
#define PLATELENGTH 7
#define ALPHABET 36
/* Characters must be uppercase or digits */
void lsd_string_sort(char* a[], const int length, int string_size) {
int R = ALPHABET;
int W = string_size;
const int N = length;
char id = 0;
char** aux = (char**)malloc(length * string_size);
for(int d = W-1; d >= 0; d--) {
int* count = (int*)calloc(R+1, sizeof(int));
/* whether character or digit */
id = (*(a[0]+d) >= '0' && *(a[0]+d) 0)
free(plates[--n]);
return 0;
}Solution
-
This macro name is misleading:
The alphabet consists of 26 letters, not 36. But in a CS context, this can be implied (in this case, it appears to include A-Z and 0-9).
However, this macro is storing the alphabet's size, not the alphabet itself. The name should clearly specify that. A more appropriate name could be
On another note, the other macro should be renamed to
-
Avoid single-character variable names:
Based on the context (fortunately), I can tell that these hold sizes. Other than that, they are very unclear and could lead to maintenance problems at some point. Always give variables descriptive names, unless they're simple loop counters.
-
You have the list-displaying code in
You could keep the label outputs in
This macro name is misleading:
#define ALPHABET 36The alphabet consists of 26 letters, not 36. But in a CS context, this can be implied (in this case, it appears to include A-Z and 0-9).
However, this macro is storing the alphabet's size, not the alphabet itself. The name should clearly specify that. A more appropriate name could be
ALPHABET_LENGTH.On another note, the other macro should be renamed to
PLATE_LENGTH. Underscores are commonly used to separate each word in a macro.-
Avoid single-character variable names:
int R = ALPHABET;
int W = string_size;Based on the context (fortunately), I can tell that these hold sizes. Other than that, they are very unclear and could lead to maintenance problems at some point. Always give variables descriptive names, unless they're simple loop counters.
-
You have the list-displaying code in
main() twice. Why not make it a function? This will reduce the amount of code in main() and maintain DRY, making it clearer to read and to maintain.void display(char list[], int n)
{
for (int i = 0; i < n; i++)
printf("%s\n", plates[i]);
}You could keep the label outputs in
main(), right before the function calls.Code Snippets
#define ALPHABET 36int R = ALPHABET;
int W = string_size;void display(char list[], int n)
{
for (int i = 0; i < n; i++)
printf("%s\n", plates[i]);
}Context
StackExchange Code Review Q#47009, answer score: 9
Revisions (0)
No revisions yet.