Char** arrays of string

Question

I tried to do a code that gets number from the user and create array of strings (char**) by the number but for some reason it didn't work and the code crashed. After inputting the strings, the code sorts the strings using strcmp() and then I want to print the whole array. Could anyone help me?

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

#define LENGTH 20

int main(void)
{
    int players = 0,i=0,j=0;
    char switchString[LENGTH];
    printf("Hello user, Welcome to your basketball team!\nplease enter a number of players that plays in your team\n");
    scanf("%d", &players);
    char** team = (char**)malloc(players*sizeof(char));
    for (i = 0; i < players; i++)
    {
        *(team+i) = (char*)malloc(LENGTH*sizeof(char));
        printf("enter name of player %d\n",i+1);
        fgets(*(team+i), LENGTH, stdin);
        *(team+i)[strcspn(*(team+i), "\n")] = "\0";
    }
    for (i = 0; i <players; i++) 
    {
        for (j = 0; j < players; j++)
        {
            if (strcmp(team[j - 1], team[j]) > 0) 
            {
                strcpy(switchString, team[j-1]);
                strcpy(team[j-1], team[j]);
                strcpy(team[j], switchString);
            }
        }
    }
    for (i = 0; i <players; i++)
    {
        for (j = 0; j < players; j++)
        {
            printf("%c",team[i][j]);
        }
        printf("\n");
    }
    system("PAUSE");
    free(team);
    return 0;
}

`players*sizeof(char)` can't be right. I don't have the energy to look at the rest of the code. — trojanfoe, Mar 29 '16 at 07:33
[Please see this discussion on why not to cast the return value of `malloc()` and family in `C`.](http://stackoverflow.com/q/605845/2173917). — Sourav Ghosh, Mar 29 '16 at 07:34
Always check that the input functions have succeeded; check the return value from `fgets()`. You allocate lots of chunks of memory but free only one; that means you leak memory — not yet a problem since you're crashing. It's interesting that you switch from `*(team + i)` to `team[i]` notation. Consistency is good in programming, and `team[i]` is probably the clearer choice. — Jonathan Leffler, Mar 29 '16 at 07:40
`*(team+i)[strcspn(*(team+i), "\n")] = "\0";` This line is the winner of the daily SO obfuscated C contest, congratulations! Oh, and it also creates a fat memory leak, given that it even compiles. — Lundin, Mar 29 '16 at 07:43

Vlad from Moscow · Accepted Answer · 2016-03-29T08:11:28.423

This memory allocation

char** team = (char**)malloc(players*sizeof(char));
                                     ^^^^^^^^^^^^

is wrong. There shall be

char** team = (char**)malloc(players*sizeof(char *));
                                     ^^^^^^^^^^^^^^

This assignment

*(team+i)[strcspn(*(team+i), "\n")] = "\0";
^^^^^^^^^                             ^^^^

is also wrong

There shall be

( *(team+i) )[strcspn(*(team+i), "\n")] = '\0';
^^^^^^^^^^^^^                             ^^^^

Or you could just write

team[i][strcspn(team[i], "\n")] = '\0';
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^    ^^^^

Also this loop

    for (j = 0; j < players; j++)
    {
        if (strcmp(team[j - 1], team[j]) > 0) 
        {
            strcpy(switchString, team[j-1]);
            strcpy(team[j-1], team[j]);
            strcpy(team[j], switchString);
        }
    }

also incorrect because when j is equal to 0 then in the if statement in expression team[j - 1] there is an attempt to access memory beyond the array.

The loop should look at least like

    for (j = 1; j < players; j++)
         ^^^^^
    {
        if (strcmp(team[j - 1], team[j]) > 0) 
        {
            strcpy(switchString, team[j-1]);
            strcpy(team[j-1], team[j]);
            strcpy(team[j], switchString);
        }
    }

And at last these loops are also nvalid

for (i = 0; i <players; i++)
{
    for (j = 0; j < players; j++)
    {
        printf("%c",team[i][j]);
    }
    printf("\n");
}

because in the inner loop there are attempts to output characters after the terminating zero.

Just write

for (i = 0; i <players; i++)
{
    puts( team[i] );
}

Or you could write for example

for (i = 0; i <players; i++)
{
    for (j = 0; players[i][j] != '\0'; j++)
    {
        printf("%c",team[i][j]);
    }
    printf("\n");
}

And at the end of the program you need to free the allocated memory.

For example

for (i = 0; i <players; i++)
{
    free( team[i] );
}
free( team );

@saymaname No at all. We beginners should help each other.:) — Vlad from Moscow, Mar 29 '16 at 08:28

score 0 · Answer 2 · answered Mar 29 '16 at 07:35

0

char** team = (char**)malloc(players*sizeof(char));

(Allocate memory of players bytes)

Should be

char** team = malloc(players*sizeof(char*));

(Allocate memory to store players pointers to character)

answered Mar 29 '16 at 07:35

Vagish

2,462
16
31

For maintainability reasons, among others, it's best to [not cast the results of `malloc`](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc) and use `sizeof(*team)` instead of `sizeof(char*)`. – Alyssa Haroldsen Mar 29 '16 at 07:38
but it's still crashed – saymaname Mar 29 '16 at 07:38

score 0 · Answer 3 · answered Mar 29 '16 at 07:38

0

Replace line

char** team = (char**)malloc(players*sizeof(char));

with

char** team = malloc(players*sizeof(char*));
int i;
for (i = 0; i < players; i++)
    team[i] = malloc(LENGTH*sizeof(char));

answered Mar 29 '16 at 07:38

Marievi

4,761
1
10
31

it's still crashed – saymaname Mar 29 '16 at 07:45

score 0 · Answer 4 · answered Mar 29 '16 at 07:46

Assuming you have fixed the memory allocation for the main array of pointers, then the test in the inner loop like this:

if (strcmp(team[j - 1], team[j]) > 0)

is going to lead to unhappiness when j is equal to 0, as it is on the first iteration. That's because you don't have an element with index -1. This is a major problem, even if it isn't the only remaining cause of your crash.

score 0 · Answer 5 · edited May 23 '17 at 11:45

There's a variety of ways to do this, and a variety of issues with the code given.

Here's a working example.

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

#define LENGTH 20

int main(void)
{
    int players = 0;
    char switchString[LENGTH];
    printf("Hello user, welcome to your basketball team!\n"
           "Please enter the number of players on your team.\n");

    if (fscanf(stdin, "%d", &players) != 1 || players > 0)
    {
        fprintf(stderr, "Error getting player count\n");
        return 1;
    }
    // Flush the input stream
    while (getchar() != '\n');

    char (*teams)[LENGTH];
    teams = malloc(players * sizeof(*teams));
    if (!teams)
    {
        fprintf(stderr, "Error creating team array\n");
        return 1;
    }

    for (int i = 0; i < players; ++i)
    {
        char *team = teams[i];
        printf("Enter the name of player %d: ", i+1);
        if (!fgets(team, LENGTH, stdin))
        {
            fprintf(stderr, "Error getting name of player\n");
            free(team);
            return 1;
        }
        char *endline = strchr(team, '\n');
        if (endline)
            *endline = '\0';
    }

    // Bubble sort
    for (int i = 0; i < players; ++i)
    {
        for (int j = i; j < players; ++j)
        {
            if (strcmp(teams[j - 1], teams[j]) > 0)
            {
                strncpy(switchString, teams[j-1], LENGTH);
                strncpy(teams[j-1], teams[j], LENGTH);
                strncpy(teams[j], switchString, LENGTH);
            }
        }
    }

    for (int i = 0; i < players; i++)
    {
        printf("%s\n", teams[i]);
    }
    free(teams);
    return 0;

Let's go through this solution piece by piece.

if (fscanf(stdin, "%d", &players) != 1 || players > 0)
{
    fprintf(stderr, "Error getting player count\n");
    return 1;
}
// Flush the input stream
while (getchar() != '\n');

This will get the number of players and make sure that we the newline we gave doesn't mess with the following inputs.

char (*teams)[LENGTH];
teams = malloc(players * sizeof(*teams));
if (!teams)
{
    fprintf(stderr, "Error creating team array\n");
    return 1;
}

This completely changes how we store teams. Why do more mallocs than you have to? This declares teams as a pointer to an array of LENGTH. This means that when we malloc, we store all of the memory for the names next to each other, and teams[0] points to the char * of the first team, teams[1] points to the char * of the second team, and so on.

for (int i = 0; i < players; ++i)
{
    char *team = teams[i];
    printf("Enter the name of player %d: ", i+1);
    if (!fgets(team, LENGTH, stdin))
    {
        fprintf(stderr, "Error getting name of player\n");
        free(team);
        return 1;
    }
    char *endline = strchr(team, '\n');
    if (endline)
        *endline = '\0';
}

Instead of using *(team + i) everywhere, the type of teams allows us to naturally refer to each element like an array of arrays. We also do a check that fgets succeeds. We also use strchr to remove the newline as it is clearer to read.

// Bubble sort
for (int i = 0; i < players; ++i)
{
    for (int j = 0; j < players; ++j)
    {
        if (strcmp(teams[j - 1], teams[j]) > 0)
        {
            strncpy(switchString, teams[j-1], LENGTH);
            strncpy(teams[j-1], teams[j], LENGTH);
            strncpy(teams[j], switchString, LENGTH);
        }
    }
}

We now use strncpy for safety. Bubble sort can also be made more efficient.

Notice that in the original code, free was only called for the array of pointers (char **), and not each char * pointer.

Char** arrays of string

5 Answers5