requesting C code feedback/review for first tiny game in CC65

[scottinNH]

Hello,

 

Could I get a code review from folks familiar with C and CC65?   https://github.com/sprive/basic-computer-games/blob/41_guess_c_fp/00_Alternate_Languages/41_Guess/guess.c

 

Am I missing out on any techniques/patterns? 

Before I jump in and port several more of the Creative Computing games to C, I want to be sure I am on-target with a good pattern...

 

My goals were:

• Cross-platform, as it's just a text game..
• Format text width to 80 or 40 columns, decided at compile time (display will just use whatever the OS text mode provides)
• Optimize a little for CC65 (minimize printf() where possible)

 

Thoughts/opinions:

• I did not see any benefit of creating a header file, because it seems to only provide an organizational benefit, which maybe is not needed with code this size.
  • I could be incorrect and I will value any feedback. 
• My manner of #ifdef print formatting seems... odd.
  • But after a little research, it seems to be the best compromise because any use of runtime "if()" checking would waste memory.
  • Also I wanted to avoid creating a CC65-specific C file since features are portable..
• Possible improvements:
  • add formatting for VIC-20 (22 column)
  • trigger CC65 support for Apple 80-column (I have a //c)

Cheers

 

Edited September 18, 2022 by scottinNH

[scottinNH]

/* INCLUDES */
#include<inttypes.h>
#include<stdint.h>
#include<stdio.h>
#include<stdlib.h>
#include<time.h>

#if defined (__CC65__)
 #include<cc65.h>
 #include<conio.h>
    //#warning "CC65 detected, so avoiding float operations."
#else
    #include<math.h> // may need to exclude if!defined _FPU_PRESENT, but need an example to test
#endif /* __CC65__ */

/* DEFINES */
//
// SCREEN_SIZE_X - Value for columns width of text screen, pass in at compile time
// Code currently has #ifdef prints for 80 or 40 columns. If you have something else (__VIC20__ is 22)
// your could extend the intro() function.
// While it is possible to call CC65's screensize() or POSIX `getenv()` that's a lot of runtime overhead for 8-bits.
#if !defined(SCREEN_SIZE_X)
    #define SCREEN_SIZE_X 80
#endif
// Stringify for debug printing defines
#define _STRINGIFY(s) #s
#define STRINGIFY(s) _STRINGIFY(s)


// Guess 
// Basic Computer Games by David Ahl (1978)
// https://www.atariarchives.org/basicgames/

// GAME NOTES FROM BOOK:
// In program GUESS, the computer chooses a random integer between 0 and any limit you set. You must
// then try to guess the number the computer has chosen using the clues provided by the computer.
//
// You should be able to guess the number in one less than the
// number of digits needed to represent the number in binary notation -- i.e., in base 2. This ought
// to give you a clue as to the optimum search technique.
//
// Guess converted from the original program in FOCAL which appeared in the book "Computers in the Classroom"
// by Walt Koetke of Lexington High School, Lexington, Massaschusetts.

// BOOK NOTES
// * Games targeted a display having 72 or 80 columns.
// * Original guess.bas had a bug where "99 END" could never be reached. This one uses inout of 0 to exit.

// C VERSION NOTES
// * Port to C by Scott Prive
// * This compiles for Atari 800 and other 6502 systems using CC65... however:
// ** v2.19 of cc65 will not optimize away printf() into puts(), doesn't have float workaround libs, or big ints.
// ** .. I had to clutter #ifdef in the middle of functions, sorry. It is a small program.
// * Coding Horror guidelines suggest mixed-case output, but UPPER feels more correct. 
//
// BUILD NOTES:
// macOS:
// /usr/bin/clang -fdiagnostics-color=always -g ./guess.c -o ./guess
// /usr/bin/clang -fdiagnostics-color=always -DDEBUG  -DSCREEN_SIZE_X=80 -g ./guess.c -o ./guess
// * CC65 (atari):
// ** cl65 -t atari -o guess.xex ./guess.c   # standard compile into bootable disk image
// ** cl65 -t atari -Ln guess.lbl --listing guess.lst -m printf.map --add-source -o guess.xex ./guess.c # make extra files
// Emulator execution:
// * atari800 -nobasic -run ./guess.xex


void intro(void);
uint16_t get_limit(void);
void instructions(uint16_t player_limit);
uint16_t player_guess(void);
void whitespace(void);
void congratulations(uint8_t guess_count, uint16_t limit_goal);
uint8_t calc_best_guess(uint16_t input);
uint8_t quit(void);
int main(void);

void intro(void)
{
    #ifdef DEBUG
        fputs("((DEBUG: SCREEN_SIZE_X="STRINGIFY(SCREEN_SIZE_X)"))\n", stdout);
    #endif
    #if SCREEN_SIZE_X == 80
        puts("                                 GUESS");
        puts("               CREATIVE COMPUTING  MORRISTOWN, NEW JERSEY");
        puts("");
        puts("");
        puts("");
        puts("THIS IS A NUMBER GUESSING GAME. I'LL THINK");
        puts("OF A NUMBER BETWEEN 1 AND YOUR LIMIT (UP TO 65535).");
        puts("THEN YOU HAVE TO GUESS WHAT IT IS.");
        puts("(TYPE 0 AT ANY TIME TO QUIT.)");
        puts("");
    #elif SCREEN_SIZE_X == 40
        puts("               GUESS");
        puts("     CREATIVE COMPUTING");
        puts("    MORRISTOWN, NEW JERSEY");
        puts("");
        puts("");
        puts("THIS IS A NUMBER GUESSING GAME.");
        puts("I'LL THINK OF A NUMBER BETWEEN 1 AND");
        puts("YOUR LIMIT (UP TO 65535).");
        puts("THEN YOU HAVE TO GUESS WHAT IT IS.");
        puts("(TYPE 0 AT ANY TIME TO QUIT.)");
        puts("");
    #endif
}

uint16_t get_limit(void)
{
    // temp var is larger and signed, so we gain room for negative and out-of-range checks
    long L;
    puts("WHAT LIMIT DO YOU WANT?");
    scanf("%lu", &L);
    if (L == 0) {
        printf("YOU ENTERED: %lu; 0 TO QUIT...\n", L);
        exit(0);
    } else if (L < 0) {
        puts("ERROR: A NEGATIVE WAS INPUT. EXITING.");
        exit(1);
    } else if (L > 65535L) {
        puts("ERROR: HIGHER THAN 65535 LIMIT WAS INPUT. EXITING.");
        exit(1);
    }
    return (uint16_t)L;
}

void instructions(uint16_t L)
{
    #if SCREEN_SIZE_X == 80
        printf("I'M THINKING OF AN NUMBER BETWEEN 1 AND %d\n", L);
        puts("NOW YOU TRY TO GUESS WHAT IT IS.");
    #elif SCREEN_SIZE_X == 40
        puts("I'M THINKING OF AN NUMBER");
        printf("BETWEEN 1 AND %d\n", L);
        puts("NOW YOU TRY TO GUESS WHAT IT IS.");
    #endif
}

uint16_t player_guess(void) 
{
    uint16_t N;
    //printf(" "); 
    scanf("%hu", &N);
    return N;
}

void whitespace(void)
{
    uint8_t h;
    for (h = 0; h<5; h++) {
        puts("");
    }
}

void congratulations(uint8_t G, uint16_t L1)
{
    #if SCREEN_SIZE_X == 80
        printf("THAT'S IT! YOU GOT IT IN %d TRIES.\n", G);
        printf("A BEST POSSIBLE SCORE (NON-LUCKY) WOULD BE %d TRIES.\n", L1);
    #elif SCREEN_SIZE_X == 40
        printf("THAT'S IT! YOU GOT IT IN %d TRIES.\n", G);
        puts("A BEST POSSIBLE SCORE (NON-LUCKY)");
        printf("WOULD BE %d TRIES.\n", L1);
    #endif
    if (G < L1) {
        puts("VERY GOOD");
    } else if (G == L1) {
        puts("GOOD");
    } else {
        #if SCREEN_SIZE_X == 80
            printf("YOU SHOULD HAVE BEEN ABLE TO GET IT IN ONLY %d.\n", L1);
        #elif SCREEN_SIZE_X == 40
            puts("YOU SHOULD HAVE BEEN ABLE");
            printf("TO GET IT IN ONLY %d.\n", L1);
        #endif
    }
}

uint8_t calc_best_guess(uint16_t input)
{
    // Get the "ideal" non-lucky guess count (ideal score).
    // See Game Notes.
    // CC65 and 6502 lack floating-port support; this scenario-specific ifdef works
    uint16_t output;
    #if defined (__CC65__)
    while (input) {
        input >>= 1;
        output ++;
    }
    #else
    output = (int)(log(input) / log(2))+1;
    #endif /* __CC65__ */
    return output;
}

uint8_t quit(void)
{
    // exit, but hold the display open on platforms that would immediately clear on exit.
    puts("GAME OVER.");
    #if defined (__CC65__)
      if (doesclrscrafterexit()){
            printf("Press any key\n");
            cgetc(); // conio.h
        }
    #endif /* __CC65__ */
    return 0;
}


int main(void)
{
    uint8_t guess_count;        // original var: G
    uint16_t player_limit;      // user-defined upper bound, inclusive. // original: L
    uint16_t limit_goal;        // An optimal guess count; score to beat.  // original var: $L1= int(log($L)/log(2))+1;
    uint16_t rand_target;       // Generated, random. Original var: M
    uint16_t user_guess;        // user-input, their guess. Original var: N
    uint8_t still_guessing;     // while condition until the game is done
    guess_count = 0;
    still_guessing = 1;

    intro();
    player_limit = get_limit();
    limit_goal = calc_best_guess(player_limit);
    instructions(player_limit);
    srand(time(NULL)); // computer's secret number
    rand_target = (rand() % player_limit) + 1; // generate "computer's guess"
    //Get input and evaluate guess
    while (still_guessing) {
        user_guess = player_guess();
        ++guess_count;
        if (user_guess < 1) {
            // bad input, start over sans computer's secret number
                whitespace();
                printf("YOU ENTERED: %d, AND 0 TO QUIT...\n", user_guess);
                exit(0);
        }  else if (user_guess <  rand_target) {
            #ifdef DEBUG
            printf("DEBUG: LAST GUESS=%d, TARGET=%d, OPTIMAL GUESS SCORE=%d, GUESSES TRIED=%d\n", user_guess, rand_target, limit_goal, guess_count);
            #endif
            puts("TOO LOW. TRY A BIGGER ANSWER.");
        } else if (user_guess > rand_target) {
            #ifdef DEBUG
            printf("DEBUG: LAST GUESS=%d, TARGET=%d, OPTIMAL GUESS SCORE=%d, GUESSES TRIED=%d\n", user_guess, rand_target, limit_goal, guess_count);
            #endif
            puts("TOO HIGH. TRY A SMALLER ANSWER.");
        } else if (user_guess == rand_target) {
            #ifdef DEBUG
            printf("DEBUG: LAST GUESS=%d, TARGET=%d, OPTIMAL GUESS SCORE=%d, GUESSES TRIED=%d\n", user_guess, rand_target, limit_goal, guess_count);
            #endif
            congratulations(guess_count, limit_goal);
            still_guessing = 0;
        } else {
            // Should not happen, but have a catch-all anyways
            puts("ERROR - Guess evaluation failure. This should not happen.");
            printf("DEBUG: LAST GUESS=%d, TARGET=%d, OPTIMAL GUESS SCORE=%d, GUESSES TRIED=%d\n", user_guess, rand_target, limit_goal, guess_count);
        }
    }
    quit();
}

 

[TGB1718]

Had a quick look through, looks ok to me, nothing jumping out 👍

[Wrathchild]

Just a couple...
The declarations of the methods aren't necessary as no forward usage is made, i.e. all are called from main which you also shouldn't need to (re)define.

The use of scanf has the first (get_limit) use a pointer to a long and the second (player_guess) to uint16_t, due to the 'lu' and 'hu'. To be consistent could the long be replaced with uint32_t ?

Edited September 19, 2022 by Wrathchild

[ilmenit]

just a minor comment - even a single use of printf or scanf will bloat your executable by a few kilobytes, because these functions are huge. Make your specialized versions for input/output.

[danwinslow]

I'll second that. If you plan on writing a larger, graphics/action game you will need to learn Atari-style minimalism. Coding for speed and memory efficiency in CC65 really requires using as little of the standard C lib as possible. It's more like using a very good macro assembler than a standard C program. Search for many good how-to's by @ilmenit in particular.

[ivop]

Even though I agree with @ilmenit and @danwinslow for larger projects, I don't think it is needed for these simple Basic conversions.

[Irgendwer]

You can write your code more maintainable and shorter if you apply once column mode depended strings and just use them afterwards:

 

...

#ifdef COLUMNS80
#define COND_SP "  "
#define COND_NL " "
#else
#define COND_SP " "
#define COND_NL "\n"
#endif

...
printf(COND_SP "Hello" COND_NL "Atari!");
...

 

if COLUMNS80 is defined this will result in

  Hello Atari!

 

else

 Hello
Atari!

 

[scottinNH]

8 hours ago, Wrathchild said:

Just a couple...
The declarations of the methods aren't necessary as no forward usage is made, i.e. all are called from main which you also shouldn't need to (re)define.

The use of scanf has the first (get_limit) use a pointer to a long and the second (player_guess) to uint16_t, due to the 'lu' and 'hu'. To be consistent could the long be replaced with uint32_t ?

Good catch. I'll try again. There's a story here...

 

When I first tried uint32_t there, I ran into all sorts of warnings when attempting to scanf() it or printf() the var.

To quiet one of the warnings, I think I needed to pass an extra field to the scanf, some defined macro from the sizes definition (?).

I still got warnings only under CC65, even though with mac gcc (clang) I specified warning checks... grrr. 

I suspect some of the solutions I was "stack overflowing my way through" only work on newer C

 

In the end it might be better to just have a warning (and know it's really OK, my OCD can handle it) 

I can be more specific on the error later

[scottinNH]

3 minutes ago, Irgendwer said:

You can write your code more maintainable and shorter if you apply once column mode depended strings and just use them afterwards:

Nice tip! The duplication was kinda bothering me. 

 

[scottinNH]

Thanks everyone... I'll start on the next one.

Further feedback still welcome, of course.

[stepho]

Generally clean code.

 

Some nit picks...

rand_target = (rand() % player_limit) + 1; // generate "computer's guess"

If player_limit is 65535 then you have a small chance of the target being 65536, which would become 0 for uint16_t .
Since you also want to eliminate 0, you probably want :

rand_target = (rand() % (player_limit-1)) + 1; // generate "computer's guess"

You can find boundary conditions like that by testing with a small limit of, say, 7 and then noticing if you occasionally get an 8.
Or by changing uint16_t to uint8_t and testing enough to see the occasional 0, then changing back to uint16_t .
Always test boundaries - its where the majority of bugs like to hide.

 

All uppercase is considered SHOUTING, which my mother taught me is rude! Just say no!
Even most old devices just display lowercase as uppercase, so it works on most old devices too.
For the very, very, few that don't display nice things for the lowercase letters, there are alternatives that I can elaborate on later.

 

Generally considered bad form to bomb out deep down inside code.
In particular, the exit() calls in get_limit() .
I would have had get_limit() return int32_t and use negative values to represent bad cases.
Then main() can decide whether to:

• bomb out doing exit(-player_limit); or better yet, calling quit();
  This can be particularly important if there is clean-up code required for some future platform.
• ask again
• substitute a sane value and play anyway.

[Harry Potter]

I'm about to SPAM you.  I would try replacing printf() with my ATASimpleIO library.  It has limited text I/O support but is much more efficient than printf() and puts().  You can find it at c65 additions - Manage /ui at SourceForge.net.  I also have a print replacement that supports printing tokens within strings.  The tokens will shorten repeated text to one byte.  It can save a little on text size.  The file is printtok_atari.c.  There is an error in the latter's docs: tokens are from 0xA0 to 0xBF, not 0x80 to 0x9F.  Try them out!

[scottinNH]

1 hour ago, Harry Potter said:

I'm about to SPAM you.  I would try replacing printf() with my ATASimpleIO library.  It has limited text I/O support but is much more efficient than printf() and puts().  You can find it at c65 additions - Manage /ui at SourceForge.net.  I also have a print replacement that supports printing tokens within strings.  The tokens will shorten repeated text to one byte.  It can save a little on text size.  The file is printtok_atari.c.  There is an error in the latter's docs: tokens are from 0xA0 to 0xBF, not 0x80 to 0x9F.  Try them out!

Neat! OK I will play around with this... it will come in handy when I get to larger games. 

I see this library (and yourself) are active in the C64 forums as well.

 

I assume license is public domain or unencumbered? 

[Harry Potter]

I thank you for your interest.    The license is public domain, but I ask you to state in your game's docs that you use my libraries.    You don't have to, though.

[scottinNH]

14 hours ago, stepho said:

Generally clean code.

 

Some nit picks...

rand_target = (rand() % player_limit) + 1; // generate "computer's guess"

If player_limit is 65535 then you have a small chance of the target being 65536, which would become 0 for uint16_t .
Since you also want to eliminate 0, you probably want :

rand_target = (rand() % (player_limit-1)) + 1; // generate "computer's guess"

You can find boundary conditions like that by testing with a small limit of, say, 7 and then noticing if you occasionally get an 8.
Or by changing uint16_t to uint8_t and testing enough to see the occasional 0, then changing back to uint16_t .
Always test boundaries - its where the majority of bugs like to hide.

 

All uppercase is considered SHOUTING, which my mother taught me is rude! Just say no!
Even most old devices just display lowercase as uppercase, so it works on most old devices too.
For the very, very, few that don't display nice things for the lowercase letters, there are alternatives that I can elaborate on later.

 

Generally considered bad form to bomb out deep down inside code.
In particular, the exit() calls in get_limit() .
I would have had get_limit() return int32_t and use negative values to represent bad cases.
Then main() can decide whether to:

• bomb out doing exit(-player_limit); or better yet, calling quit();
  This can be particularly important if there is clean-up code required for some future platform.
• ask again
• substitute a sane value and play anyway.

Awesome feedback, thanks.

And I agree the code will be much cleaner and reusable if the function just checks the thing, without it making surprise decisions for whatever uses the function 🙂

 

The uppercase is emotional for me - it's how I typed these in 40 years ago on the A8.. it feels era-appropriate even though the A8 has lower.

On Jeff Atwood's (Coding Horror) site he too asked people to be open-minded about supporting mixed case, but I think 90% of submissions we

At the risk of over-engineering (for practice), I'll define both upper and lower strings in another file

 

Does anyone write unit or integration tests for CC65 apps?  I can check everything manually, but I haven't really written tests in C.

[Harry Potter]

If you want help to optimize your program, try reading my cc65 optimizations at c65 additions - Manage Files at SourceForge.net.