Allocating memory by using Malloc (char & int)

Question

I'm new to C , and I'm trying to understand malloc. I'm trying to create a program that assigns memory for cards/colors and print them out.

I've made a function that looks like this:

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

#define ACE 1;
#define CardSize 52
#define colors 4

int main() {
    count();
    system("pause");
    return 0;
}

void count() {
    int *cards;
    int i, j, f;
    char *color[4] = { "Diamond", "Heart", "Spade", "Clubs"};
    cards = malloc(CardSize * sizeof(int));
    *color = malloc(colors * sizeof(char)); //Here's where my program crashes
    for (f = 0; f < 4; f++) {
        for (i = 0; i < 13; i++) {
            cards[i] = (i % 13) + 1;
            printf("%d of %s\n", cards[i], color[f]);
        }
    }
}

Without the line *color = malloc(colors*sizeof(char)); the program works fine, but I want to allocate memory for my colors.

The output is this:

1 of ════════════════════════════════════════════════════²²²²╬─Ép┐p
2 of ════════════════════════════════════════════════════²²²²╬─Ép┐p
3 of ════════════════════════════════════════════════════²²²²╬─Ép┐p
4 of ════════════════════════════════════════════════════²²²²╬─Ép┐p
5 of ════════════════════════════════════════════════════²²²²╬─Ép┐p
6 of ════════════════════════════════════════════════════²²²²╬─Ép┐p
7 of ════════════════════════════════════════════════════²²²²╬─Ép┐p
8 of ════════════════════════════════════════════════════²²²²╬─Ép┐p
9 of ════════════════════════════════════════════════════²²²²╬─Ép┐p
10 of ════════════════════════════════════════════════════²²²²╬─Ép┐p
11 of ════════════════════════════════════════════════════²²²²╬─Ép┐p
12 of ════════════════════════════════════════════════════²²²²╬─Ép┐p
13 of ════════════════════════════════════════════════════²²²²╬─Ép┐p

Which should be diamonds, then the rest is printed out fine, 1 of hearts, 2 of hearts, etc and all the other colors.

Please can you help me understand where I'm making the mistake, and what I'm doing wrong?

Aside: although you don't use ACE yet, you should probably remove the ; at the end of #define ACE 1; — Weather Vane
– Weather Vane, Commented Dec 20, 2015 at 23:05
Why do you need to allocate and assign another memory while there are already (pointers to first elements of) string literals assigned? malloc() doesn't initialize allocated memory, so the printf() will print randam data, and maybe cause Segmentation Fault if there isn't terminating null character before what you cannot read. What do you want to do? — MikeCAT
– MikeCAT, Commented Dec 20, 2015 at 23:06
What do you expect *color = malloc(colors*sizeof(char)); to do? — Stack Exchange Broke The Law
– Stack Exchange Broke The Law, Commented Dec 20, 2015 at 23:13
You already allocated memory for the colour names. Why do you want to allocate more memory? Especially in light of your comment " i just wanted to specificly store just the amount of memory that was necessary." — M.M
– M.M, Commented Dec 21, 2015 at 0:04

chqrlie · Accepted Answer · 2015-12-20 23:21:03Z

2

You define color to be an array of 4 pointers to char arrays that you initialize correctly:

char *color[4] = { "Diamon", "Heart", "Spade", "Clubs"};

But you immediately store another pointer in the first element of this array:

*color = malloc(colors*sizeof(char)); //Here's where my program crashes

You should not do that at all. I don't really understand your intention in doing so, but just remove this line and your program will behave correctly. Note that string literals are stored in read only memory, so color should really be defined as:

const char *color[4] = { "Diamon", "Heart", "Spade", "Clubs" };

Also fix the typo on Diamond, Hearts and Spades. Indeed the suits are plural except Diamond.

The value for card should probably be computed differently, as a number from 0 to 51. Also cards should be returned or freed if you do not use it. The code would be changed this way:

void count(void) {
    int *cards = malloc(CardSize * sizeof(int));
    int i, j, f;
    const char *color[4] = { "Diamond", "Hearts", "Spades", "Clubs" };

    for (i = 0; i < CardSize; i++) {
        j = (i % 13) + 1;  // Card value
        f = (i / 13) % 4;  // color number 0 to 3
        cards[i] = i % 52;
        printf("%d of %s\n", j, color[f]);
    }
    free(cards);
}

edited Dec 20, 2015 at 23:21

answered Dec 20, 2015 at 23:11

chqrlie

152k12 gold badges145 silver badges231 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

William hart Over a year ago

But really, why do you use f = (i / 13) % 4; , cant you just skip that line, the output will be the same if you'll do it my way anyways? Thank you for replying! But, the reason why i wanted to use malloc is that i just wanted to specificly store just the amount of memory that was necessary. By the way, when im going to shuffle my deck, can i still use the same variables from the count() function?

chqrlie Over a year ago

@Williamhart: I should have written f = (i % 52) / 4;. The reason I do this is for the count function to work even if you are dealing more than 52 cards. Your version uses the implied assumption that CardSize == 52. If you modify the #define for some reason, the code will either deal too many cards and access memory beyond the allocated amount or fail to deal more than 52 cards.

chqrlie Over a year ago

Implied assumptions, especially involving explicit constants is bad practice. They often cause errors, especially when modifying a program to extend functionality. Try and avoid them.

MikeCAT · Accepted Answer · 2015-12-20 23:20:39Z

0

These extra allocations are for nothing but exercise, but try this.

void count() {
    int *cards;
    int i, j, f;
    const char *color_data[4] = { "Diamon", "Heart", "Spade", "Clubs"};
    char **color;
    cards = malloc(CardSize*sizeof(int));
    color = malloc(colors*sizeof(char*));
    for (i = 0; i < 4; i++) {
        color[i] = malloc((strlen(color_data[i]) + 1)*sizeof(char));
        strcpy(color[i], color_data[i]);
    }
    for (f = 0; f < 4; f++) {
        for (i = 0; i < 13; i++) {
            cards[i] = (i % 13) + 1;
            printf("%d of %s\n", cards[i], color[f]);
        }
    }
    free(cards);
    for (i = 0; i < 4; i++) {
        free(color[i]);
    }
    free(color);
}

Notes:

Add #include <string.h> to the head of your code to use strlen() and strcpy().
Omitted here, the return values of malloc() should be checked for not being NULL.
Multiplying by sizeof(char) (=1) means nothing, but I kept it for not losing readability.
Avoid memory leaks. Use free() for memories allocated via malloc().

answered Dec 20, 2015 at 23:20

MikeCAT

75.1k11 gold badges50 silver badges71 bronze badges

2 Comments

William hart Over a year ago

Thank you for replying! But, the reason why i wanted to use malloc is that i just wanted to specificly store just the amount of memory that was necessary. By the way, when im going to shuffle my deck, can i still use the same variables from the count() function?

MikeCAT Over a year ago

What do you mean? You bring back no variables from count() so far. If you implement shuffle in count(), yes, you can use these variables, but then the name count will be confusing.

Collectives™ on Stack Overflow

Allocating memory by using Malloc (char & int)

2 Answers 2

3 Comments

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

3 Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related