Algoritmo ASCII Random Walk

Question

Vorrei qualche consiglio su come migliorare / ottimizzare un piccolo programma che ho scritto. Quello che fa è eseguire una passeggiata casuale su un collettore toroidale con 8 colori che girano intorno.

//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

Osservazioni generali

Benvenuti alla revisione del codice, bella prima domanda. Tralascerei le informazioni sulla licenza poiché lo stack exchange utilizza la licenza Creative Commons Attribution-ShareAlike .

Il blocco dei commenti in alto è piuttosto utile altrimenti. Cordiali saluti, questo si compila bene su Windows 10 in Visual Studio 2019 Professional, ma non si collega ( usleep()non è definito).

Parte del codice è un po 'difficile da leggere a causa dell'affollamento verticale, potresti voler inserire alcune righe vuote all'interno main()tra i blocchi logici.

Evita le variabili globali

È molto difficile leggere, scrivere, eseguire il debug e mantenere programmi che utilizzano variabili globali. Le variabili globali possono essere modificate da qualsiasi funzione all'interno del programma e quindi richiedono che ciascuna funzione venga esaminata prima di apportare modifiche al codice. In C e C ++ le variabili globali influiscono sullo spazio dei nomi e possono causare errori di collegamento se sono definite in più file. Le risposte in questa domanda di stackoverflow forniscono una spiegazione più completa.

Dichiarare e inizializzare le variabili dove sono necessarie

Nella versione originale di C le variabili dovevano essere dichiarate all'inizio del blocco logico in cui erano usate, questo non è più vero, dichiarale come necessarie.

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) {
        ...
    }
}

Lascia che sia il compilatore a fare di più

Nell'esempio di codice precedente non è stata definita alcuna dimensione per l'array inst[], il compilatore lo inserirà in base al numero di valori di inizializzazione nell'array. Ciò semplifica la scrittura e la manutenzione del codice poiché la dimensione non deve essere modificata ogni volta che viene aggiunta un'istruzione.

Numeri magici

Ci sono numeri magici nella main()funzione (336 e 67), potrebbe essere meglio creare costanti simboliche per rendere il codice più leggibile e più facile da mantenere. Hai già definito costanti simboliche per TAPE_SIZE_Xe TAPE_SIZE_Yil codice sarebbe più leggibile e comprensibile se i valori di inizializzazione per xe yusassero costanti simboliche, in questo caso potresti eventualmente rendere la metà di inizializzazione della dimensione del nastro per xe y.

Le costanti numeriche nel codice sono a volte indicate come numeri magici , perché non hanno un significato ovvio per loro. C'è una discussione su questo su stackoverflow .

Complessità

La funzione main()è troppo complessa (fa troppo). Man mano che le dimensioni dei programmi crescono, l'uso di main()dovrebbe essere limitato a chiamare funzioni che analizzano la riga di comando, chiamare funzioni che sono impostate per l'elaborazione, chiamare funzioni che eseguono la funzione desiderata del programma e chiamare funzioni per ripulire dopo la parte principale di il programma.

C'è anche un principio di programmazione chiamato Principio di responsabilità unica che si applica qui. Il principio di responsabilità unica afferma:

che ogni modulo, classe o funzione dovrebbe avere la responsabilità su una singola parte della funzionalità fornita dal software e tale responsabilità dovrebbe essere interamente incapsulata da quel modulo, classe o funzione.

Ci sono almeno più funzioni in main().

• // TODO: implementare l'input del programma e i comandi condizionali
• Imposta lo schermo
• Anche i contenuti del while (1)ciclo sembrano essere molteplici funzioni

Casi predefiniti per switchdichiarazioni

Sebbene non sia necessario nel codice corrente, per evitare comportamenti indefiniti è meglio avere un caso predefinito per ogni istruzione switch:

        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;
        }

Possibile ottimizzazione

La seconda switchistruzione potrebbe essere sostituita con una tabella di valori e ciò migliorerebbe le prestazioni.

        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

Leggibilità e manutenibilità

Come accennato nelle osservazioni generali, questa sezione di codice è molto difficile da leggere e ancora più difficile da mantenere:

        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 linguaggi come C, C ++ o Java inserire il codice di azione come x = 0;sulla propria riga, preferibilmente tra parentesi graffe, che rende l'azione più facile da trovare e con le parentesi graffe più facili da mantenere aggiungendo più istruzioni in seguito:

        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

Un esempio di quanto sia difficile leggere il codice originale è che ho perso il numero magico 8nella discussione sui numeri magici sopra.

Answer 2

Non usare system()per compiti banali

Chiamare system()significa avviare un nuovo processo di shell, che a sua volta deve analizzare il comando ed eseguirlo. Il clearcomando non è integrato in Bash, quindi la shell a sua volta inizierà un nuovo processo da eseguire /usr/bin/clear. Ed è solo un programma, non è magia; clearè anche scritto in C. E tutto ciò che alla fine fa è semplicemente stampare una sequenza di escape, come già fai nel tuo programma. Puoi sostituire system("clear")con:

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

Considera l'idea di utilizzare ncurses

Stai facendo affidamento su sequenze di escape che, come hai già menzionato nei commenti, non funzionano su tutti i sistemi. Fortunatamente, esiste una libreria che fornisce questa funzionalità in modo multipiattaforma (e multipiattaforma): ncurses . In effetti, il clearcomando che hai chiamato fa parte del pacchetto ncurses. Ma è anche meglio, ncurses ti consente di spostare il cursore in posizioni arbitrarie, cambiare i colori e così via.

Evita di ripeterti

Ogni volta che ripeti la stessa cosa due o più volte, dovresti trovare un modo per automatizzarla. Ad esempio, puoi sostituire la seconda switchaffermazione con:

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

Crea funzioni per problemi secondari.

Anche se non ti ripeti, potrebbe avere senso creare una nuova funzione per gestire un piccolo problema. Questo di solito rende il codice più facile da leggere. Ad esempio, deformi e blocca le coordinate xe ydopo aver spostato la posizione, potresti creare funzioni per farlo, ad esempio:

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

E usalo in questo modo:

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

Non codificare le dimensioni dello schermo

Quando ho provato a eseguire il tuo programma, all'inizio pensavo fosse difettoso, ma si è scoperto che richiede un terminale di 672 per 135 caratteri. Trova un modo per ottenere la dimensione corrente del terminale. Potresti usare atoi(getenv("COLUMNS"))e atoi(getenv("LINES"))su sistemi operativi tipo UNIX, ma ancora una volta usare una libreria come ncurses si prenderà cura di questo per te.

Esempio completo

Di seguito è riportato il tuo programma, ma utilizzando ncurses. Poiché ncursestiene già traccia del contenuto dello schermo, incluso il colore utilizzato in ciascuna posizione, non è più necessario l'array tape[][].

Come accennato da pacmaninbw, evita anche di codificare i numeri e / #defineo dichiarali come static const intinvece.

#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();
}