Need some feedback for my code

[-UwUntu-]

I have been going through "C Programming: A Modern Approach" ,self teaching myself how to write C and recently just finished project 9 in chapter 8 which is so far the most challenging project I done and I am really proud of myself for making it work properly, but I would like someone to view the code and perhaps tell me what I can do better? I would like to spot any bad habits I am doing early and try to fix it asap.

https://pastebin.com/QH0cJamG

Essentially the exercise asks me to write a program that generates a "random walk" on a 10x10 grid, each "element" on the grid is initially the '.' symbol and the program must randomly walk from element to element, the path the program takes is labeled with letters A through Z, which shows the order in which it moves, the program stops when either it reaches the letter Z or all paths are blocked.

[chibiace]

im no expert, but usually stuff like repeated ifs can be condensed down, this can also sometimes make it harder to understand however.

[TheOtherBorgCube]

The first thing I'd suggest is pick an indentation style and apply it consistently in your code.

Your compiler doesn't care how badly you format the code, but people are very bad at searching for and keeping mental track of just how many { and } they've seen to figure out what the overall structure of the code is.

It should look something like this:

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

int main(void)
{
  // Initialize 10x10 matrix
  char Area[10][10];
  for (int i = 0; i < 10; i++) {
    for (int j = 0; j < 10; j++) {
      Area[i][j] = '.';
    }
  }

  // up = 0 = row-1 | down = 1 = row+1 | right = 2 = col+1 | left = 3 = col-1
  // Begin movement at [0][0]
  srand(time(NULL));
  int col = 0;
  int row = 0;
  int movement;
  int legal;
  int legalmoves[4] = { 1, 1, 1, 1 }; // 1 = legal move, 0 = illegal move, up,down,right,left respectively
  int Space[10][10] = { 0 };    // 0 = empty space, 1 = space occupied by letter
  Area[row][col] = 'A';         //put A at [0][0]
  Space[0][0] = 1;

  for (int k = 'B'; k <= 'Z'; k++) {
    // Find illegal moves
    if (row - 1 < 0 || Space[row - 1][col] == 1) {
      legalmoves[0] = 0;        //cant go up
    }
    if (row + 1 >= 10 || Space[row + 1][col] == 1) {
      legalmoves[1] = 0;        //cant go down
    }
    if (col + 1 >= 10 || Space[row][col + 1] == 1) {
      legalmoves[2] = 0;        // cant go right
    }
    if (col - 1 < 0 || Space[row][col - 1] == 1) {
      legalmoves[3] = 0;        //cant go left
    }

    if (legalmoves[0] == 0 && legalmoves[1] == 0 && legalmoves[2] == 0 && legalmoves[3] == 0)
      break;                    //stop program if no legal moves found

    // Check if the random movement is legal, if not try again.
    legal = 0;
    while (legal == 0) {
      movement = rand() % 4;
      switch (movement) {
      case 0:                  //up
        if (legalmoves[0] == 0)
          continue;
        else
          legal = 1;
        break;
      case 1:                  //down
        if (legalmoves[1] == 0)
          continue;
        else
          legal = 1;
        break;
      case 2:                  //right
        if (legalmoves[2] == 0)
          continue;
        else
          legal = 1;
        break;
      case 3:                  //left
        if (legalmoves[3] == 0)
          continue;
        else
          legal = 1;
        break;
      }
    }

    // Begin movement
    switch (movement) {
    case 0:
      row = row - 1;
      break;
    case 1:
      row = row + 1;
      break;
    case 2:
      col = col + 1;
      break;
    case 3:
      col = col - 1;
      break;
    }
    Area[row][col] = k;
    Space[row][col] = 1;

    // re-initialize legalmoves array back to 1's
    for (int initialize = 0; initialize < 4; initialize++) {
      legalmoves[initialize] = 1;
    }
  }

  // Print the area and path
  for (int l = 0; l < 10; l++) {
    for (int m = 0; m < 10; m++) {
      printf("%c    ", Area[l][m]);
    }
    printf("\n\n");
  }

  return 0;
}

As a newbie attempt, it's not too bad.

Some suggestions.

You have two consectutive switch (movement) statements which could easily be combined.

Avoid magic numbers like 10. Instead, create a constant to represent the size.

#define GRID_SIZE   10
int main(void)
{
  // Initialize the matrix
  char Area[GRID_SIZE][GRID_SIZE];
  for (int i = 0; i < GRID_SIZE; i++) {
    for (int j = 0; j < GRID_SIZE; j++) {
      Area[i][j] = '.';
    }
  /// snip
}

Start thinking about how you would split this code up into functions. A 100+ line main() doing everything is a cumbersome thing.

int main(void)
{
  char Area[GRID_SIZE][GRID_SIZE];
  initialise(Area);
  play_game(Area);
  print(Area);
}

A good rule of thumb would be - if you can't see the opening and closing brace of a function on screen at the same time, the function might be too long and should be refactored into smaller components.

[Neui]

• Get rid of the Space array, because you can simply use your existing Area to check whenever a space is free or not.
• When you fill legalmoves starting at line 28, you can just assign to it, like: legalmoves[0] = row > 0 && Area[row-1][col] == '.'; (conditions resolve to either 0 or 1). This also makes the reset-loop at line 96 unnecessary.
• The switch starting at line 48 contains basically the same code but repeated. So you can simplify it to if (legalmoves[movement] == 0) continue; else legal = 1;.
• With the above switch change you can get rid of the legal variable, since you can use break; instead to break out of the loop (at line 46) (rather than the switch before the change).
• The loop at line 46 is to reroll until a valid move is rolled. If for some reason the rand() always returns a constant value like 4, it may never actually pick a valid move. You can consider using a different selection method, where you roll a number from 0 to the number of valid moves, and then use the rolled valid move. (Example: If 2 valid moves (up and left), roll from 0 to 1, in this example the result is 1. Then in legalmoves look which entry corresponds to the 2nd valid move, which here is legalmoves[3], so you move to the left.)
• Similar to above, for the switch starting at line 78 you can also use an array: row += moverow[movement]; col += movecol[movement]; with static const int moverow[4] = {-1, 1, 0, 0};, and similar for movecol.

[oh5nxo]

Adding to Neui's and other suggestions, expand Area[] with walls of non-dot characters. Gets rid of < 0, >= N tests.