C++

Forum

 
Review & Comments on my Random Walk

gn4cys (4)
Hello,

I came up with exercise for myself in which i tried to use most of my current C++ knowledge with which i feel comfortable (well, i've understood concept of class recently and that gave me ability to write something more advanced :)). I decided to write random walk cmd application working on 2D grid. I consider this done in about 90% (this is just exercise i came up for myself), however there are still few things which i am unhappy about or simply do not understand.

Whole program was written almost from scratch, using some reference to Tic Tac Toe game to figure out how can i create 2D grid. I list my questions and code below, also, i am very open for commentaries about application of solutions (I did not checked how exactly people written their Random Walks in C++ to make it more of a challenge for myself) and also about how could i do it better. I realize that certain things are not complete i.e Error Checking, however i am more interested now in general quality of design (for lack of a better word).

Short description: Program takes seed value from which walk should start based on supplied X,Y coordinates. Then it makes fixed amount of moves (10 in code below, i tested input for less and up to 50), then it asks if users wants another generation.

My questions:
//1. How to efficiently print bigger grid without defining each element individually?

//2. How to stop characters from going outside of scope of 2D grid, for example X=12,Y=11 if there is only 10x10. Why does it even happens that x,y can be more than 10 or less than 0?

//3. How can i avoid "jumping" (and why exactly this happens?) from one edge of grid to another, for example, if X=10,Y=10, character @ in some instances jumps to X=1,Y=1 which is not a wanted behavior for random walk.

//4. Overall comments to code, is it messy solutions i applied? How can i make it less messy? I used here most of my current knowledge in c++ with which i feel comfortable, the only thing i did not use is vectors, and i thought about applying them to somehow keep track of position of @ character and then rolling-back to some value with place to move if it will try to go out-of-scope of 2D grid

//5. Once a while, depending on given seed value and amount of requested generation code seems to crash. Why? I know it has something to do with exponentially growing difference in X,Y coordinates, for example X=-182 Y=1. This connects with my previous question.


1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
 
#include <iostream>
#include <stdio.h>
#include <stdlib.h>
#include <time.h>
#include <vector>

using namespace std;

class Grid{
public:
    Grid();
    void MoveCharacter();
    int counter = 0;
    int seed_x, seed_y;

private:
    char board [10][10];
    void ClearBoard();
    void PrintBoard();
    int movingPositionX();
    int movingPositionY();
    int randomCopy();
    void SimulateCharacter();

};

Grid::Grid()
{
    ClearBoard();
}


void Grid::ClearBoard()
{
    for (int i = 0; i < 10; i++)
    {
        for (int j = 0; j < 10; j++)
        {
            board[i][j] = ' ';
        }
    }
}

void Grid::PrintBoard() // How can i efficiently print more elements, for example 100x100?
{
    cout << " 1 2 3 4 5 6 7 8 9 10\n";
    for (int i = 0; i < 10; i++)
    {
        ++counter;
        cout << "-----------------------\n";
        cout << i+1 << board[0][i] << "|" << board[1][i] << "|" << board[2][i] << "|" << board[3][i] << "|" << board[4][i] << "|"
        << board[5][i] << "|" << board[6][i] << "|" << board[7][i] << "|" << board[8][i] << "|" << board[9][i] << "|\n";
    }
    cout << "-----------------------\n";

}

void Grid::MoveCharacter()
{
    char player = '@';
    bool exec = false;

    int x,y;

    while (exec == false){
            PrintBoard();
            cout << "Enter seed value separated by newline\n";
            cout << "X: ";
            cin >> x;
            cout << "Y: ";
            cin >> y;
            x = x - 1;
            y = y - 1;
            seed_x = x;
            seed_y = y;
            board[x][y] = player;
            exec = true;
    }

    SimulateCharacter();

}

void Grid::SimulateCharacter()
{
    bool walk = false;
    char player = '@';
    int walk_x,walk_y;

    while (walk == false){

        for (int i = 0; i < 10; i++){

            PrintBoard();
            walk_x = movingPositionX();
            walk_y = movingPositionY();
            board[walk_x][walk_y] = player;

            if (walk_x > 9 || walk_y > 9 || walk_x < 1 || walk_y < 1){ // If walk tries to go out-of-scope of defined grid, i am using seed value to restart
                walk_x = seed_x;
                walk_y = seed_y;
            }

            if (i == 9){
                walk = true;
                string restart;
                cout << "Next Generation(y)?\n";
                cin >> restart;
                if (restart == "y"){
                    //ClearBoard();
                    walk = false;
                    cout << walk_x << " " << walk_y << "\n"; // Just for reference to know printed values
                } else {
                    cout << "Thank You";
                    walk = true;
                }

            }

            }


        }
}

Grid::movingPositionX()
{

    if (counter == 0) {
        randomCopy();
    }

    int x = rand() % 10;
    ++counter;
    int walk_x;

    if (x < 5){
      ++seed_x;
      walk_x = seed_x;
      if (walk_x >= 9) seed_x - --walk_x;
      if (walk_x <= 1) seed_x - ++walk_x;
      return walk_x;
    } else if (x > 5 || x == 5) {
      --seed_x;
      walk_x = seed_x;
      if (walk_x >= 9) seed_x - --walk_x;
      if (walk_x <= 1) seed_x - ++walk_x;
      return walk_x;
    }

}


Grid::movingPositionY()
{
    if (counter == 0) {
        randomCopy();
    }

    int y = rand() % 10;
    ++counter;
    int walk_y;

    if (y < 5){
      ++seed_y;
      walk_y = seed_y;
      if (walk_y >= 9) seed_y - --walk_y;
      if (walk_y <= 1) seed_y + ++walk_y;
      return walk_y;
    } else if (y > 5 || y == 5) {
      --seed_y;
      walk_y = seed_y;
      if (walk_y >= 9) seed_y - --walk_y;
      if (walk_y <= 1) seed_y + ++walk_y;
      return walk_y;
    }

}

Grid::randomCopy()
{
    srand(time(nullptr));
}

int main()
{

Grid ObjectGrid;
ObjectGrid.MoveCharacter();

return 0;


}
Last edited on
TheIdeasMan (6781)
Hi,

Consider splitting your code into header and cpp files, like to use *.hpp for c++ headers. In a larger program this will allow reuse of code.

Don't have using namespace std; Google as to why not, and what to do, there is plenty written about it.

Don't have public data member variables. Instead make an interface of functions to interact with the data. This doesn't necessarily mean blindly having a get and set function for each member.

To print out a 2d array, have a nested for loop like you have in the ClearBoard function. Also have a const or constexpr variable which is the size of the grid.

Functions which don't change the state of the class (don't change the values of any members) should be marked const before the opening brace. Note that 2 functions that differ only in const or not, are 2 different functions. This is one example of const correctness and it a good thing because the compiler can help you. Use const or constexpr as much as you can, including function arguments / parameters that shouldn't change.

Consider using unsigned types, there shouldn't ever be a possibility for negative values especially for array subscripts. There is a type std::size_t which is the largest unsigned type that the system has, these days typically 64 bit. It is also the type used by STL containers, such as std::vector etc.

Turn on as many warnings in the compiler as is feasible. I used the cpp.sh (the gear icon top right of the code) withall 3 warnings on, and had this output:

126:23: error: ISO C++ forbids declaration of 'movingPositionX' with no type [-fpermissive]
In member function 'int Grid::movingPositionX()':
140:31: warning: value computed is not used [-Wunused-value] 141:31: warning: value computed is not used [-Wunused-value] 146:31: warning: value computed is not used [-Wunused-value] 147:31: warning: value computed is not used [-Wunused-value]
At global scope:
154:23: error: ISO C++ forbids declaration of 'movingPositionY' with no type [-fpermissive]
In member function 'int Grid::movingPositionY()':
167:31: warning: value computed is not used [-Wunused-value] 168:31: warning: value computed is not used [-Wunused-value] 173:31: warning: value computed is not used [-Wunused-value] 174:31: warning: value computed is not used [-Wunused-value]
At global scope:
180:18: error: ISO C++ forbids declaration of 'randomCopy' with no type [-fpermissive]
In member function 'int Grid::randomCopy()': 183:1: warning: no return statement in function returning non-void [-Wreturn-type]
In member function 'int Grid::movingPositionX()':
151:1: warning: control reaches end of non-void function [-Wreturn-type]
In member function 'int Grid::movingPositionY()':
178:1: warning: control reaches end of non-void function [-Wreturn-type]

Warnings are your friend, they tell you about problems with your code.

On lines 140, 141, 146, 147 you need an assignment statement.

There you go some things to work on :+)

Good Luck
TheIdeasMan (6781)
Answers to some of the other questions:

2. You can use modulus operator (%) to prevent your number larger than the size or the grid, to help initially, then code to test if a move is still inside the grid.

1
2
3
 
constexpr std::size_t GridSize {10}; // brace initialisation
short RandomNum = 15; // short int - we could get a negative here. You will have a function call to get the random number
short MoveAmount = GridSize % RandomNum; // Now between -9 and +9


You write a function to do this. I used short here but one obliviously has to choose a type that is big enough

3. You have to decide what is going to happen if a move goes outside the grid. One could adopt the maximum or minimum for each X or Y ordinate. So if the start is 2,2 the move is 9,5 the result is 9,7 for a 10 by 10 grid - which has maximum array subscript of 9,9.

5. Fix the compile errors / warnings, that is why warnings are your friend. When I am coding I am not finished until there are no warnings, not even pedantic warnings. You should look at the documentation for your IDE, set the warnings to their highest reasonable level - sometimes a compiler can report too many, including from code in system header files. One has to be very careful not to exceed the array subscripts (you should write code to ensure this), and another golden rule is to always initialise the variables, ideally wait until you have a sensible value to assign, then do declaration and assignment all in 1 statement, like I have done with the code above.
gn4cys (4)
Thank You,

There is definitely a lot to learn when it comes to good practices. I corrected some of the things You mentioned. Some of things i do not understand, maybe because i am still beginner. I am posting changed code and, if You could be kind enough, for some additional tips.

1. I am still keeping this code in one file just because it is easier for now to operate on it. I know how to split it into different files, i will do it for final version. This is a tip i see very often.
2. Deleted namespace std.

Don't have public data member variables. Instead make an interface of functions to interact with the data. This doesn't necessarily mean blindly having a get and set function for each member.


Could you explain in a little more detail? I understand that this is about seed_x,seed_y variables in public. How can i properly change it?


To print out a 2d array, have a nested for loop like you have in the ClearBoard function. Also have a const or constexpr variable which is the size of the grid.


Somehow i still have a problem with that. I tried using nested loop like in ClearBoard function, it does not work. Character @ does not want to move above 10x10 or it simply does not print at all on grid. Another option is that i get only vertical grid, but not horizontal, or @ is printed only in first column. I will not post code of all of my different tries because it takes to much of a space. Could i ask for some example on how can i generate grid of, let's say 100x100 cells?

Functions which don't change the state of the class (don't change the values of any members) should be marked const before the opening brace. Note that 2 functions that differ only in const or not, are 2 different functions. This is one example of const correctness and it a good thing because the compiler can help you. Use const or constexpr as much as you can, including function arguments / parameters that shouldn't change.


That is very useful tip. I tried changing function definition to const here, however then i get a lot of pedantic errors (without this, code does not produce any errors on my side). So for the time being i've kept it as is.

Consider using unsigned types, there shouldn't ever be a possibility for negative values especially for array subscripts.


Could You show me an example?

I've stopped generation of numbers above 10 or below 0 by changing my if statements inside of a movingPositionX(or Y) function so it would be called again if x or y will not meet this criteria. Somehow i feel that this is a dirty hack because code needs to call itself with unwanted variable as long as it will not find the correct one. But, it works and my code stopped to crash now.

To summarize, unsolved for me is still:
1. How can i make grid 100x100 in PrintBoard(). (I feel stupid because of it, but i totally does not understand why i fail here)
2. @ character jumping somehow from i.e 10,10 to 10,1. Imagine if this random walk would be a population of Africa. After reaching edges of African continent suddenly they would appear on the far edge of North America. This is wrong.

Could You have a look now?

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
 
#include <iostream>
#include <stdio.h>
#include <stdlib.h>
#include <time.h>
#include <vector>

class Grid{
public:
    Grid();
    void MoveCharacter();
    int counter = 0;
    int seed_x, seed_y;

private:
    char board [10][10];
    void ClearBoard();
    void PrintBoard();
    int movingPositionX();
    int movingPositionY();
    int randomCopy();
    void SimulateCharacter();

};

Grid::Grid()
{
    ClearBoard();
}


void Grid::ClearBoard()
{
    for (int i = 0; i < 10; i++) //rows
    {
        for (int j = 0; j < 10; j++) //columns
        {
            board[i][j] = ' ';
        }
    }
}

void Grid::PrintBoard() // How can i efficiently print more elements, for example 100x100?
{
    std::cout << " 1 2 3 4 5 6 7 8 9 10\n";
    for (int i = 0; i < 10; i++)
    {
        ++counter;
        std::cout << "-----------------------\n";
        std::cout << i+1 << board[0][i] << "|" << board[1][i] << "|" << board[2][i] 
        << "|" << board[3][i] << "|" << board[4][i] << "|"
        << board[5][i] << "|" << board[6][i] << "|" << board[7][i] << "|" 
        << board[8][i] << "|" << board[9][i] << "|\n";
    }
    std::cout << "-----------------------\n";


}

void Grid::MoveCharacter()
{
    char player = '@';
    bool exec = false;

    int x,y;

    while (exec == false){
            PrintBoard();
            std::cout << "\nEnter seed value separated by newline\n";
            std::cout << "X: ";
            std::cin >> x;
            std::cout << "Y: ";
            std::cin >> y;
            x = x - 1;
            y = y - 1;
            seed_x = x;
            seed_y = y;
            board[x][y] = player;
            exec = true;
    }

    SimulateCharacter();

}

void Grid::SimulateCharacter()
{
    bool walk = false;
    char player = '@';
    int walk_x,walk_y;

    while (walk == false){

        for (int i = 0; i < 3; i++){

            PrintBoard();
            walk_x = movingPositionX();
            walk_y = movingPositionY();
            board[walk_x][walk_y] = player;

            if (i == 2){
                walk = true;
                std::string restart;
                std::cout << "Next Generation(y)?\n";
                std::cout << walk_x << " " << walk_y << "\n"; // Just for reference to know printed values
                std::cin >> restart;
                if (restart == "y"){
                    walk = false;
                } else {
                    std::cout << "Thank You";
                    walk = true;
                }

            }

            }


        }
}

int Grid::movingPositionX()
{

    if (counter == 0) {
        randomCopy();
    }

    int x = rand() % 10;
    ++counter;
    int walk_x;

    if (x < 5){
      ++seed_x;
      walk_x = seed_x;
      if (walk_x >= 10 || walk_x < 1){ // Here we check if generated walk value does not exceed.
        movingPositionX();
      }
      return walk_x;
    } else if (x > 5 || x == 5) {
      --seed_x;
      walk_x = seed_x;
      if (walk_x >= 10 || walk_x < 1){ // Here we check if generated walk value does not exceed.
        movingPositionX();
      }
      return walk_x;
    }

    return 0;

}


int Grid::movingPositionY()
{
    if (counter == 0) {
        randomCopy();
    }

    int y = rand() % 10;
    ++counter;
    int walk_y;

    if (y < 5){
      ++seed_y;
      walk_y = seed_y;
      if (walk_y >= 10 || walk_y < 1){ // Here we check if generated walk value does not exceed.
        movingPositionY();
      }
      return walk_y;
    } else if (y > 5 || y == 5) {
      --seed_y;
      walk_y = seed_y;
      if (walk_y >= 10 || walk_y < 1){ // Here we check if generated walk value does not exceed.
        movingPositionY();
      }
      return walk_y;
    }

    return 0;


}

int Grid::randomCopy()
{
    srand(time(nullptr));
    return 0;
}

int main()
{

Grid ObjectGrid;
ObjectGrid.MoveCharacter();

return 0;


}



Last edited on
TheIdeasMan (6781)
Could you explain in a little more detail? I understand that this is about seed_x,seed_y variables in public. How can i properly change it?


Put them in the private section of the class. If they are only accessed by other functions in the same class you don't have to do anything else. By an interface of functions, I mean that when one designs a class they should decide what data should be exposed to the outside world, and how the user of the class is going to interact with that data. So one creates class functions to do that. But don't blindly create a get / set function for each member variable, they might as well all be public ! :+) For example say you have a Triangle class: I wouldn't have a get / set function for each of the 3 points, instead I would functions like Move, Rotate, Scale.

Could i ask for some example on how can i generate grid of, let's say 100x100 cells?


1
2
3
4
5
6
7
8
9
10
11
12
13
 
void Grid::PrintBoard(const std::size_t BoardSize) const // How can i efficiently print more elements, for example 100x100?
{
    std::cout << " 1 2 3 4 5 6 7 8 9 10\n";
    for (std::size_t Row = 0; Row < BoardSize; Row++) {
       for (std::size_t Row = 0; Col < BoardSize; Col++) {
            std::cout << i+1 << board[Row][Col] << " | ";
       }
    std::cout << "\n";
    }
    std::cout << "-----------------------\n";


}


Somehow i still have a problem with that. I tried using nested loop like in ClearBoard function, it does not work. Character @ does not want to move above 10x10 or it simply does not print at all on grid. Another option is that i get only vertical grid, but not horizontal, or @ is printed only in first column. I will not post code of all of my different tries because it takes to much of a space.


Your question conflates moving with printing. In your movingPosition functions you don't set the position to its min or max value (0 or 9). Calling the function again recursively doesn't help. The values you check with are too big, the last subscript is 9.

That is very useful tip. I tried changing function definition to const here, however then i get a lot of pedantic errors (without this, code does not produce any errors on my side). So for the time being i've kept it as is.


Note you can only use it if the function does not change the value of any member variable. I put it on the PrintBoard
function. The function Clearboard could have it too. If a const functions attempts to call another non const function or change the value of a member then it's an error, that is why it's a good thing.

Could You show me an example?


An array or other container such as std::vector must have a positive value for a subscript, so we use an unsigned type for that. I used std::size_t which is the largest unsigned type the system has. You could declare your array with that too:


1
2
3
 
private:
constexpr Size = 10;
    char board [Size][Size];


constexpr is stronger than const because it is set in stone at compile time, it can't be cast to non const. And it does expressions as in:

constexpr double Tau = 2.0 * 3.14159;

There are other uses for it too. You will need your compiler to do at least the C++11 standard for constexpr.

Good Luck !!

Topic archived. No new replies allowed.