0

I have to code a battleship game for school so i'm trying to generate the map on which the players are going to place their boats, i have a segfault but i don't see why.

Here's my code :

Main :

int main(int ac, char **av)
{
    map_data map_data;
    print_map(map_gen(map_data));
    return(0);
}

Struct :

typedef struct map_data {
    int lines;
    int letters;
    int width;
    int height;
}map_data;

Map generation :

char **map_gen(map_data map_data)
{
    map_data.lines = 0;
    map_data.width = 18;
    map_data.height = 10;
    map_data.letters = 65;
    char **map = malloc(sizeof((char)18 * 10));

    for (int s = 0; s <= map_data.height ; s++) {
        for (int c = 0; c <= map_data.width; c++) {
            map_fill(map, map_data, s, c);
        }
    }
    return (map);
}

Filling of the char **map :

void char_fill(char **map, char ch, int s, int c)
{
        map[s][c] = ch;
}

void map_fill(char **map, map_data map_data, int s, int c)
{
    if (c == 1)
        char_fill(map, '|', s, c);
    if (s == 1)
        char_fill(map, '-', s, c);
    if (c == map_data.width)
        char_fill(map, '\n', s, c);
    map_fill2(map, map_data, s, c);
}

int map_fill2(char **map, map_data map_data, int s, int c)
{
    if (s == 0 && c == 0)
        char_fill(map, ' ', s, c);
    if (s == 1 && c == 1)
        char_fill(map, '+', s, c);
    if (c > 1 && c % 2 == 0)
        char_fill(map, '.', s, c);
    if (s > 1 && c % 2 == 1)
        char_fill(map, ' ', s, c);
    if (s == 10 && c == 18)
        char_fill(map, '\0', s, c);
    if (s > 1 && c == 0) {
        char_fill(map, my_int_to_char(map_data.lines), 0, 0);
        map_data.lines = map_data.lines + 1;
    }
    if (!map[s][c]) {
        my_putstr("error filling the map, please try again.");
        return (EXIT_FAILURE);
    }
}

Print :

void print_map(char **map)
{
    int h = 0;
    int w = 0;

    while (map[h][w] != '\0') {
        while (map[h][w] != '\n') {
            my_putchar(map[h][w]);
            w = w + 1;
        }
        h = h + 1;
    }
}

Am i doing something wrong ? Any tips on how to improve my code ?

Thanks

Aoqe
  • 1

2 Answers2

1
  1. you do not need a pointer to pointer only 2D array for it.
  2. Use the correct types.
  3. Wrap the data into your structure. Do noy use separate data structures without need
  4. Indexes in C atart from 0.
typedef struct map_data {
    size_t lines;
    size_t letters;
    size_t width;
    size_t height;
    char map[];
}map_data;

int map_fill2(map_data *map, size_t s, size_t c);


void char_fill(map_data *map, char ch, size_t s, size_t c)
{
    char (*cmap)[map -> width] = (char (*)[map -> width])map -> map;
    cmap[s][c] = ch;
}

void map_fill(map_data *map, size_t s, size_t c)
{
    if (c == 0)
        char_fill(map, '|', s, c);
    if (s == 0)
        char_fill(map, '-', s, c);
    if (c == map -> width - 1)
        char_fill(map, '\n', s, c);
    map_fill2(map, s, c);
}

int map_fill2(map_data *map, size_t s, size_t c)
{
    char (*cmap)[map -> width] = (char (*)[map -> width])map -> map;
    if (s == 0 && c == 0)
        char_fill(map, ' ', s, c);
    if (s == 1 && c == 1)
        char_fill(map, '+', s, c);
    if (c > 1 && c % 2 == 0)
        char_fill(map, '.', s, c);
    if (s > 1 && c % 2 == 1)
        char_fill(map, ' ', s, c);
    if (s == 10 && c == 18)
        char_fill(map, '\0', s, c);
    if (s > 1 && c == 0) {
        char_fill(map, my_int_to_char(map -> lines), 0, 0);
        map -> lines += 1;
    }
    if (!cmap[s][c]) {
        puts("error filling the map, please try again.");
        return (EXIT_FAILURE);
    }
    return 0;
}

map_data *map_gen(size_t lines, size_t letters, size_t width, size_t height)
{
    map_data *map = malloc(sizeof(*map) + width * height * sizeof(map -> map[0]));

    if(map)
    {
        map -> width = width;
        map -> lines = lines;
        map -> letters = letters;
        map -> height = height;
        for (size_t s = 0; s < height ; s++) 
        {
            for (size_t c = 0; c < width; c++) 
            {
                map_fill(map, s, c);
            }
        }
    }
    return (map);
}
0___________
  • 60,014
  • 4
  • 34
  • 74
  • Might want to add a refence to [Flexible array member](https://en.wikipedia.org/wiki/Flexible_array_member) in this answer's good application of it. Likely something new for OP. – chux - Reinstate Monica Dec 30 '21 at 23:22
  • thank you for your time, what you wrote has truly been helpful to help me understand. – Aoqe Dec 31 '21 at 00:48
0

At least this issue:

sizeof((char)18 * 10) is the sizeof an int, likely 4.

Allocate to the size of the referenced object times the number needed.

// char **map = malloc(sizeof((char)18 * 10));
char **map = malloc(sizeof *map * map_data.height);

Then allocate for each row

// Note <, not <=
for (int row = 0; row < map_data.height; row++) {
    map[row] = malloc(sizeof *(map[row]) * map_data.width);
    for (int col = 0; col < map_data.width; col++) {
        map_fill(map, map_data, row, col);
    }
}

Robust code would also check for allocation failures.

chux - Reinstate Monica
  • 143,097
  • 13
  • 135
  • 256
  • 2
    a bit fanatic attempt to avoid `()` in `sizeof`. It makes t hard to read. `sizeof(*map) * map_data.height` is much much better – 0___________ Dec 30 '21 at 22:49
  • @0___________ Neither fanatic nor hard to read, but a style issue. Like such issues that tend to a holy war, code to your group's style guide. Perhaps `sizeof map[0] * map_data.height`? – chux - Reinstate Monica Dec 30 '21 at 22:59
  • @0___________ [this](https://stackoverflow.com/q/21065124/2410359) goes into the `()` a bit. C spec uses all 3: `sizeof *ptr, sizeof ptr[0], sizeof (*ptr)`. – chux - Reinstate Monica Dec 30 '21 at 23:04
  • 1
    What does that link proof? IMO `sizeof *map * map_data.height` is not readable even if it is correct in C. `()` provide logic separation between object and multiplication making it much easier to read and maintain in the future – 0___________ Dec 30 '21 at 23:11
  • @0___________ Link is a reference to others views, not a proof, nor implied to be one. – chux - Reinstate Monica Dec 30 '21 at 23:18