Question

Can anyone refactor my code? its a randwalk implementation: http://ideone.com/iPBNj

Answer 1

what's wrong with the code that you're trying to fix?

Answer 2

I don't know. Maybe it can be made shorter?

Answer 3

so it does a random walk until it either reaches the edge or loops back on itself?

Answer 4

It functions perfectly... I think; it's doing exactly what it's supposed to do, which is to print a field of characters which does a random walk, stopping whenever the guy get's enclosed by his own path.

Answer 5

ok, i think i see, if he picks a direction that's off the board or onto itself it loosp back and picks another direction

Answer 6

oh and it only moves N, S, E, and W.

Answer 7

right

Answer 8

basically it isn't allowed to walk on places it's already been

Answer 9

well if you want general C style tips, my #1 cardinal rule is to avoid "copy and paste", i.e. you should only write each piece of code exactly once. if you find yourself ever saying to yourself, "if I ever change X, i have to remember to also change Y," then you've done something wrong.

Answer 10

right now all your cases share code in common, that is, checking for '.' and setting the termination condition. that can all be factored out. The only thing the case statement should do is set a candidate new X and Y. then after that, back in the common code path, you decide if you've found a valid spot, or if you should go back and try again.

Answer 11

so something like:
int new_x, new_y;

switch (rand() % 4) {
case 0: new_x = x; new_y = y+1; break;
case 1: new_x = x; new_y = y-1; break;
case 2: new_x = x+1; new_y = y; break;
case 3: new_x = x-1; new_y = y; break;
}

Then after, you can check new_x/new_y to make sure it is inside your matrix bounds and contains a '.'.

Answer 12

that makes it shorter :D

Answer 13

That would shorten that do-while loop to below 20 lines.

Answer 14

then you can say
if (new_x >= 0 && new_y >= 0 &&& new_x < 10 && new_y < 10 && field[new_x][new_y] == '.' {
field[new_x][new_y] = nextletter;
x = new_x;
y = new_y;
} else {
//c heck for boxed-in, otherwise
// loop around;
}

Answer 15

so the other thing i'd suggest, also following my copy-paste rule, is that you replace everywhere you have the constant "10" with a #define

e.g. #define MATRIX_SIZE 10

Answer 16

i've been programming at this point for, gee, almost 30 years, and the one rule that is more important than all others is that one

Answer 17

the moment you find yourself thinking "if i ever change X, i have to remember to change Y too", you're doing something wrong

Answer 18

should I make it a #define or can I just say const int MATRIXSIZE = 10 or use enum?

Answer 19

well you don't want it to be an enum, since that's meant just to differentiate different cases from each other. and const int MATRIXSIZE is a good idea if you're using C++, i don't think that's in standard C though. maybe they added it in c99, I don't remember. it has the advantage of being type-checked.

Answer 20

so I have the option of
#define MATRIXSIZE 10
const int MATRIXSIZE = 10;
enum {MATRIXSIZE = 10};

and each one works quite differently :(

Answer 21

well i wouldn't use the enum way, that's meant for another use and could have negative effects. const int is the best, #define is second best depending on if your compiler supports it.

Answer 22

#define MATRIXSIZE 10 literally just does a copy-and-paste at compile time. const int MATRIXSIZE acts more like a variable - it is typechecked, for example.

Answer 23

what if I go with the enum and use that?

Answer 24

i wouldn't do that, enums are not meant to be constants with a fixed value.

Answer 25

they usually have vlaues assigned automatically by the compiler - giving it a number to use as a constant is weird, not the intended use.

Answer 26

That #define MATRIXSIZE 10 thing makes the randwalk thing easier to deal with; now I can make my field be a square of whatever size by simply editing one number

Answer 27

exactly