ASCII Random Walk Algorithm

Question

I would like some advice on improving/optimizing a small program I wrote. What it does is run a random walk on a torus manifold with 8 colors that loop around.

//RTM Test Using Random Walk Fractals, WIP
//Created by delta23
//This Code works on Minix, Unix, MacOSX, Linux
//This Code does not work on DOS or Windows (yet...)
//LICENSE OF CODE: CC0 - Public Domain
#include <stdio.h>
#include <stdlib.h>
#include <time.h>
#define TAPE_SIZE_X 672
#define TAPE_SIZE_Y 135
//Q!><^V became ><^V : move and add
char inst [ 4 ] = { '>', '<', '^', 'V' } ; //can be used for debugging, and for inputting code later on, when it starts in a conditional loop perhaps
unsigned char tape [ TAPE_SIZE_X ] [ TAPE_SIZE_Y ] ;
int x = 336 ;
int y = 67 ;
void main ( void ) {
 system ( "clear" ) ; //clear screen
 srand ( time ( NULL ) ) ; //initialize random generator; TODO: port to C++ and use TRNG instead
 printf ( "\e[?25l" ) ; //disable cursor
 //TODO: Implement program input and conditional commands
 while ( 1 ) {
 switch ( inst [ rand ( ) % 4 ] ) { //MOVE
 case '>' :
 x = x + 1 ;
 break ;
 case '<' :
 x = x - 1 ;
 break ;
 case '^' :
 y = y + 1 ;
 break ;
 case 'V' :
 y = y - 1 ;
 break ;
 }
 if ( x >= TAPE_SIZE_X ) x = 0 ; //TOROID CODES FOR OOB CURSOR
 else if ( x < 0 ) x = TAPE_SIZE_X - 1 ;
 if ( y >= TAPE_SIZE_Y ) y = 0 ;
 else if ( y < 0 ) y = TAPE_SIZE_Y - 1 ;
 tape [ x ] [ y ] = tape [ x ] [ y ] + 1 ; // ADD 1
 if ( tape [ x ] [ y ] == 8 ) tape [ x ] [ y ] = 0 ;
 usleep ( 16667 ) ; // CAP SPEED TO 60FPS : 1/60 seconds = 16666.666... microseconds
 printf ( "33円[%d;%dH", y, x ) ; //print @ cursor position
 switch ( tape [ x ] [ y ] ) { //Color Code for Mem
 case 0 :
 printf ( "\x1B[40m " ) ;
 break ;
 case 1 :
 printf ( "\x1B[41m " ) ;
 break ;
 case 2 :
 printf ( "\x1B[43m " ) ;
 break ;
 case 3 :
 printf ( "\x1B[42m " ) ;
 break ;
 case 4 :
 printf ( "\x1B[46m " ) ;
 break ;
 case 5 :
 printf ( "\x1B[44m " ) ;
 break ;
 case 6 :
 printf ( "\x1B[45m " ) ;
 break ;
 case 7 :
 printf ( "\x1B[47m " ) ;
 break ;
 }
 fflush ( stdout ) ; //flush stdout to prevent abnormal lag in printing to screen
 }
}
```

Answer 1

General Observations

Welcome to code review, nice first question. I would leave the licensing information out since stack exchange uses the Creative Commons Attribution-ShareAlike license.

The comment block at the top is rather helpful otherwise. FYI, this compiles fine on Windows 10 in Visual Studio 2019 Professional, but doesn't link (usleep() is undefined).

Some of the code is a little difficult to read as there is vertical crowding, you might want to insert a few blank lines inside main() between logic blocks.

Avoid Global Variables

It is very difficult to read, write, debug and maintain programs that use global variables. Global variables can be modified by any function within the program and therefore require each function to be examined before making changes in the code. In C and C++ global variables impact the namespace and they can cause linking errors if they are defined in multiple files. The answers in this stackoverflow question provide a fuller explanation.

Declare and Initialize the Variables Where They are Needed

In the original version of C variables needed to be declared at the top of the logic block where they were used, this is no longer true, declare them as necessary.

void main(void) {
 char inst[] =
 {
 '>',
 '<',
 '^',
 'V'
 };
 system("clear"); //clear screen
 srand(time(NULL)); //initialize random generator; TODO: port to C++ and use TRNG instead
 printf("\e[?25l"); //disable cursor
 //TODO: Implement program input and conditional commands
 unsigned char tape[TAPE_SIZE_X][TAPE_SIZE_Y];
 int x = 336;
 int y = 67;
 while (1) {
 ...
 }
}

Let the Compiler Do More of the Work

In the code example above there is no size defined for the array inst[], the compiler will fill this in based on the number of initialization values in the array. This makes it easier to write and maintain the code since the size doesn't need to be modified each time an instruction is added.

Magic Numbers

There are Magic Numbers in the main() function (336 and 67), it might be better to create symbolic constants for them to make the code more readable and easier to maintain. You already define symbolic constants for TAPE_SIZE_X and TAPE_SIZE_Y the code would be more readable and understandable if the initialization values for x and y used symbolic constants, in this case you could possibly make the initialization half of the tape size for x and y.

Numeric constants in code are sometimes referred to as Magic Numbers, because there is no obvious meaning for them. There is a discussion of this on stackoverflow.

Complexity

The function main() is too complex (does too much). As programs grow in size the use of main() should be limited to calling functions that parse the command line, calling functions that set up for processing, calling functions that execute the desired function of the program, and calling functions to clean up after the main portion of the program.

There is also a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states:

that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

There are at least multiple functions in main().

• //TODO: Implement program input and conditional commands
• Set up the screen
• The contents of the while (1) loop also seem to be multiple functions

Default Cases for switch Statements

While it isn't necessary in the current code, to prevent undefined behavior it is best to have a default case for every switch statement:

 switch (inst[rand() % 4]) { //MOVE
 case '>':
 x = x + 1;
 break;
 case '<':
 x = x - 1;
 break;
 case '^':
 y = y + 1;
 break;
 case 'V':
 y = y - 1;
 break;
 default :
 printf("Unknown instruction in switch\n");
 return 1;
 }

Possible Optimization

The second switch statement could be replaced with a table of values and that would improve performance.

 char* control_sequence[] =
 {
 "\x1B[40m ",
 "\x1B[41m ",
 "\x1B[43m ",
 "\x1B[42m ",
 "\x1B[46m ",
 "\x1B[44m ",
 "\x1B[45m ",
 "\x1B[47m ",
 };
 printf("%s", control_sequence[tape[x][y]]);
 fflush(stdout); //flush stdout to prevent abnormal lag in printing to screen

Readability and Maintainability

As mentioned in the general observations, this section of code is very hard to read and even harder to maintain:

 if (x >= TAPE_SIZE_X) x = 0; //TOROID CODES FOR OOB CURSOR
 else if (x < 0) x = TAPE_SIZE_X - 1;
 if (y >= TAPE_SIZE_Y) y = 0;
 else if (y < 0) y = TAPE_SIZE_Y - 1;
 tape[x][y] = tape[x][y] + 1; // ADD 1
 if (tape[x][y] == 8) tape[x][y] = 0;
 usleep(16667); // CAP SPEED TO 60FPS : 1/60 seconds = 16666.666... microseconds
 printf("33円[%d;%dH", y, x); //print @ cursor position

In languages like C, C++ or Java put action code such as x = 0; on its own line, preferably in braces, that makes the action easier to find, and with the braces easier to maintain by adding more statements later:

 if (x >= TAPE_SIZE_X)
 {
 x = 0;
 }
 else if (x < 0)
 {
 x = TAPE_SIZE_X - 1;
 }
 if (y >= TAPE_SIZE_Y)
 {
 y = 0;
 }
 else if (y < 0)
 {
 y = TAPE_SIZE_Y - 1;
 }
 tape[x][y] = tape[x][y] + 1; // ADD 1
 if (tape[x][y] == 8)
 {
 tape[x][y] = 0;
 }
 usleep(16667); // CAP SPEED TO 60FPS : 1/60 seconds = 16666.666... microseconds
 printf("33円[%d;%dH", y, x); //print @ cursor position

An example of how hard the original code is to read is that I missed the magic number 8 in the discussion of magic numbers above.

Answer 2

Don't use system() for trivial tasks

Calling system() means starting a new shell process, which in turn has to parse the command and execute it. The clear command is not a built-in for Bash, so the shell in turn will start a new process to execute /usr/bin/clear. And it's just a program, it's not magic; clear itself is also written in C. And all it eventually does is just print an escape sequence, like you already do in your program. You can replace system("clear") with:

printf("\e[1;1H\e[2J");

Consider using ncurses

You are relying on escape sequences that, as you already mention in the comments, don't work on every system. Luckily, there's a library that provides this functionality in a cross-platform (and cross-terminal) way: ncurses. In fact, the clear command you called is part of the ncurses package. But it's even better, ncurses allows you to move the cursor to arbitrary positions, change colors, and so on.

Avoid repeating yourself

Whenever you are repeating the same thing twice or more times, you should find a way to automate it. For example, you can replace the second switch-statement with:

printf("\x1B[%dm ", 40 + tape[x][y]);

Create functions for sub-problems.

Even if you don't repeat yourself, it might make sense to create a new function to handle a small problem. This usually makes the code more easy to read. For example, you warp and clamp the x and y coordinates after moving the position, you could create functions to do this, for example:

static int wrap(int value, int limit) {
 if (value < 0)
 return limit - 1;
 else if (value >= limit)
 return 0;
 else
 return value;
}

And use it like so:

switch(...) {
case '>':
 x = wrap(x + 1, TAPE_SIZE_X);
 break;
case '<':
 x = wrap(x - 1, TAPE_SIZE_X);
 break;
...

Don't hardcode the screen size

When I tried to run your program, I thought it was buggy at first, but it turns out it requires a terminal that is 672 by 135 characters big. Find some way to get the current size of the terminal. You could use atoi(getenv("COLUMNS")) and atoi(getenv("LINES")) on UNIX-like operating systems, but again using a library like ncurses will take care of this for you.

Complete example

Below is your program, but using ncurses. Since ncurses already tracks the contents of the screen, including the color used at each position, it no longer needs the array tape[][].

As pacmaninbw mentioned, also avoid hardcoding numbers, and #define or declare them as static const int instead.

#include <ncurses.h>
#include <stdlib.h>
#include <time.h>
static int clamp(int value, int limit) {
 if (value < 0)
 return 0;
 else if (value >= limit)
 return limit - 1;
 else
 return value;
}
static int wrap(int value, int limit) {
 if (value < 0)
 return limit - 1;
 else if (value >= limit)
 return 0;
 else
 return value;
}
void main(void)
{
 srand(time(NULL));
 /* Initialize the screen and colors */
 initscr();
 curs_set(0);
 start_color();
 static const int max_colors = 8;
 for (int i = 0; i < max_colors; i++)
 init_pair(i, COLOR_WHITE, i);
 int x = COLS / 2;
 int y = LINES / 2;
 /* Set the delay for keyboard input */
 static const int framerate_limit = 60; /* Hz */
 timeout(1000 /* ms */ / fraterate_limit);
 /* Main loop */
 while (getch() == ERR) {
 switch (rand() % 4) {
 case 0: x = wrap(x + 1, COLS); break;
 case 1: x = wrap(x - 1, COLS); break;
 case 2: y = clamp(y + 1, LINES); break;
 case 3: y = clamp(y - 1, LINES); break;
 }
 int cur_color = PAIR_NUMBER(mvinch(y, x));
 int new_color = wrap(cur_color + 1, max_colors);
 chgat(1, A_NORMAL, new_color, NULL);
 refresh();
 }
 endwin();
}

• chux: timeout(1000 / 60) lacks time units context. Perhaps #define KDB_DELAY_ms (1000/60) timeout(KDB_DELAY_ms); or simply timeout(1000 /* ms */ / 60)?