Trying to change element in 2D array in C

Question

I'm suppose to implement in the function make2Darray to make the elements in the 2D array correspond with the row number.

The other parts of the code that prints and frees was already given, so no need to worry about that. I'm only suppose to touch the make2Darray function. However, in that function, the allocating part was also given. So the only code I am to alter is the part where I change the elements in the 2D array.

int** make2Darray(int width, int height) {
  int **a;
  int i = 0;
  int j = 0;

  /*allocate memory to store pointers for each row*/
  a = (int **)calloc(height, sizeof(int *));
  if(a != NULL) {
    /* allocate memory to store data for each row*/
    for(i = 0; i < height; i++) {
      a[i] = (int *)calloc(width, sizeof(int));
      if(a[i] == NULL) {
        /* clean up */
        free2Darray(a, height);
        return NULL; /*aborting here*/
      }
    }
  }
    /* from this point down is the part I implemented, all code above was 
    given*/
    if (height < 0 && width < 0) {
      for (i = 0; i < height; i++) {
        for (j = 0; j < width; j++) {
          a[i][j] = j;
        }
      }
    }
    return a;
}

The elements in the 2D array is suppose to correspond to the row number If height = 4 and width = 3

However, I always get 0s which was the default setting when I got the code

You have `/* allocate memory to store data for each row?` but don't have `*/` at the end. The comment continues for a few lines, commenting out critical code in the process. Did you really intend to do this? — Craig Estey, Feb 01 '19 at 08:11
oh, my bad. That was just a typo since I retyped the code on the forum. I went back and fixed it, thank you. — Lorelai, Feb 01 '19 at 08:14
"if (height < 0 && width < 0)". It would be to late to check for negative values at this point. Your program has probably already crashed. Even if the condition somehow holds (by miracle or by abundance of dumb luck), the next line is a no-op because `i < height` never holds. You *probably* wanted to say `if (height > 0 && width > 0)` (or perhaps `>=`) but it makes little sense at this point, as the `for` loop in effect does the necessary check. You should have checked it before the first call to `calloc`. — n. m. could be an AI, Feb 01 '19 at 08:15
" I retyped the code on the forum". Never do this. Copy and paste. If you don't have your code with you, open an online compiler, type your code there, verify it does what you say it does, and then copy and paste from there. — n. m. could be an AI, Feb 01 '19 at 08:18
As others have mentioned, `if (height < 0 && width < 0)` is probably wrong and should be corrected. But, even it's been corrected, I don't think you need it. The nested `for` loops should work fine without it. — Craig Estey, Feb 01 '19 at 08:18
Welcome to Stack Overflow! [Do I cast the result of malloc?](https://stackoverflow.com/q/605845/2173917) — Sourav Ghosh, Feb 01 '19 at 08:18
@n.m. Thanks for the feedback. I'm on my 2nd week of learning C at school and I don't really know how to do anything much. We use MobaXterm for our code and I had no idea how to copy the code. But yes, I'll use an online computer next time. — Lorelai, Feb 01 '19 at 08:26
@LKim it's online compiler, not computer. [here's one](https://repl.it/repls/ElderlySmartLightweightprocess). — Sourav Ghosh, Feb 01 '19 at 08:27
Just learn how to copy and paste with mobaxterm, it's not hard :) — n. m. could be an AI, Feb 01 '19 at 08:29

Support Ukraine · Accepted Answer · 2019-02-01T08:59:36.063

You have two major problems in the code.

1) The code where you initialize the 2D array shall be inside the if-block

2) if (height < 0 && width < 0) { is wrong - you want > instead of <

Try:

int** make2Darray(int width, int height) {
  int **a;
  int i = 0;
  int j = 0;

  /*allocate memory to store pointers for each row*/
  a = (int **)calloc(height, sizeof(int *));
  if(a != NULL) {
    /* allocate memory to store data for each row*/
    for(i = 0; i < height; i++) {
      a[i] = (int *)calloc(width, sizeof(int));
      if(a[i] == NULL) {
        /* clean up */
        free2Darray(a, height);
        return NULL; /*aborting here*/
      }
    }

    // Moved inside the if(a != NULL) {

    /* from this point down is the part I implemented, all code above was 
    given*/
    if (height > 0 && width > 0) {   // Corrected this part 
      for (i = 0; i < height; i++) {
        for (j = 0; j < width; j++) {
          a[i][j] = j;
        }
      }
    }

  }
  return a;
}

A few hint:

1) Do the check for height and width in the beginning of your function - like:

if (height <= 0 || width <= 0) return NULL;

2) The prototype make2Darray(int width, int height) seems backward to me as we normally mention row-count before column-count. I would prefer: make2Darray(int height, int width). I even prefer the term "row" instead of "height" and "column" instead "width".

3) Your current code do "all the real stuff" inside the if(a != NULL) { That's okay but the code would (to me) be more clear if you instead did if(a == NULL) return NULL;

4) No need to cast calloc

With these updates the code could be:

int** make2Darray(int rows, int columns) {
  int **a;
  int i = 0;
  int j = 0;

  if (rows <= 0 || columns <= 0) return NULL;
  a = calloc(rows, sizeof(int*));
  if(a == NULL) return NULL;

  /* allocate memory to store data for each row*/
  for(i = 0; i < rows; i++) {
      a[i] = calloc(columns, sizeof(int));
      if(a[i] == NULL) {
        /* clean up */
        free2Darray(a, rows);
        return NULL; /*aborting here*/
      }
  }

  /* from this point down is the part I implemented, all code above was 
     given*/
  for (i = 0; i < rows; i++) {
      for (j = 0; j < columns; j++) {
          a[i][j] = j;
      }
  }

  return a;
}

Sourav Ghosh · Answer 2 · 2019-02-01T08:16:14.937

0

In your code, the if check seems out of place.

  if(a != NULL) {
    /* allocate memory to store data for each row?
    for(i = 0; i < height; i++) {
      a[i] = (int *)calloc(width, sizeof(int));
      if(a[i] == NULL) {
        /* clean up */
        free2Darray(a, height);
        return NULL; /*aborting here*/
      }

Well, that makes no sense. In case of success, a is supposed to be not equal to NULL.

You should rather have a check for a == NULL and subsequently return NULL;, not the other way around.

That said:

if (height < 0 && width < 0) does not look very good. It's most likely wrong (in this usage context) and counter productive.
All the access for a[i][j] is invalid. You just have the space allocated for the int *s, the int *s themselves do not point to any valid memory. You need to allocate memory to them also (for example, using the width as size).

edited Feb 01 '19 at 08:16

answered Feb 01 '19 at 08:09

Sourav Ghosh

133,132
16
183
261

Even then, the array will never be populated. See my comment above. – Federico klez Culloca Feb 01 '19 at 08:11
@FedericoklezCulloca I was expanding my answer, thanks for commenting. – Sourav Ghosh Feb 01 '19 at 08:13
Sorry to nitpick but that `if (height < 0 && width < 0)` is not meaningless. It's outright wrong. If the point is to fill the array, it will *never* be filled when that condition is present. – Federico klez Culloca Feb 01 '19 at 08:14
@FedericoklezCulloca That is what I meant by counter-productive. I'll remove the "meaningless", if that removes confusion. – Sourav Ghosh Feb 01 '19 at 08:15

Trying to change element in 2D array in C

2 Answers2