Anyone able to help a beginner?

Hi,
I am a beginner to C, and after learning some stuff thought I'd try and write a simple adventure game. I haven't done much of it yet, only basic interaction, but am wondering how to improve the code so far (especially memory management!). It is on github here:
https://github.com/Joshun/adventure/

This is my first attempt at making something, so don't mock me too much if it is terrible (likely)!

Thanks

Joshun

Always free the memory when you no longer need it. Here, it may pose no problem at all, but it will come to haunt you if you try to reuse the code in a different way.

Thanks for the tip. I wanted to free the memory (when the program ends, just before

Code:

---

return 0

---

), but it just segfaults - any idea how to resolve this? This is on a Linux-based operating system.
Code from adventure.c:

Code:

---


// Text Adventure v1 by Joshua O'Leary


// Preprocessor Directives
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "definitions.h"


// Debug Mode when set to 1
#define DEBUG 1


#define INPUTLEN 20


#define PROMPT "\nWhat now?"
//extern struct gameplay status;


//void initial_state()
//{
//    status.gameover = 0; status.room = 2;
//}


int main()
{   
    //initial_state();
    char *input = malloc(INPUTLEN * sizeof(char));
    printf("\nAdventure v1");
    printf("\nYou are outside the house");
    while ( status.gameover == 0 )
    {
        prompt(&input);
    }
    return 0;
}


void control(char value)
{
    switch (value)
    {
        case 'q':
        printf("\nExiting...");
        status.gameover = 1;
        break;
    }


}


void prompt(char *input)
{   
    printf("%s", PROMPT);
    gets(input);
    //printf("%c", *input);
    //input++;
    //printf("%c", *input);
    analyse(&input);
}


void analyse(char **input)
{
    #if DEBUG == 1
        printf("\nInput: %s", *input);
        printf("\nRoom: %d", status.room);
    #endif
    switch ( **input )
    {
        case 'Q':
        case 'q':
            printf("\nquit called");
            control('q');
            break;
        case 'N':
        case 'n':
            gotoroom(0);
            break;
        case 'S':
        case 's':
            gotoroom(1);
            break;
        case 'E':
        case 'e':
            gotoroom(2);
            break;
        case 'W':
        case 'w':
            gotoroom(3);
            break;
        default :
            printf("\n?!");
            break;
    }
}

---

rooms.c (kept separate because there may be long descriptions:

Code:

---


#include <stdio.h>
#include "definitions.h"


void gotoroom(int direction)
{


    // Room Descriptions
    const char *descript[] = { "You are outside the house",
                                "You are in the front room",
                                "You are on the door-mat" };


    // Room Maps
    const int map[][4] = { { 1, -1, -1, -1 }, /* { north, south, east, west } */
                          { 2, 0, 3, 2 },    /*  a negative value means you can't move there */
                          { -1, 1, 1, 3 } };
    //printf("%d", map[status.room][direction]);
    //printf("%d", status.room);
    if ( map[status.room][direction] < 0 )
    {
        printf("\nYou can\'t go there");
    }
    else
    {
        status.room = map[status.room][direction];
        printf("\n%s", descript[status.room]);
    }
}

---

And of course definitions.h:

Code:

---


void control(char value);
void prompt();
void analyse(char **input);
void gotoroom(int direction);


struct gameplay
{
    int room;
    int gameover;
    int hasitems[5];
} status;

---

Code:

---

prompt(&input);

---

change the above to this

Code:

---

prompt(input);

---

because input is already a char pointer ;)

Aslo welcome to the forum :D
(which is not Greek despite the fact that two Greeks answered to you :p )

Code:

---

gets(input);

---

No, no, no....a thousand times no!

FAQ > Why gets() is bad / Buffer Overflows - Cprogramming.com

Code:

---

void analyse(char **input)

---

There's absolutely no need to use char** here. Just pass a single char. Unless you're planning on expanding this to use more than one char, in which case it should be

Code:

---

void analyse(const char *input)

---

Thanks std10093, that cleared things up. Those pesky pointers, it is always hard to remember the input format especially with char pointers (which sometimes require the addressof operator). Thanks for welcoming me too :)

Apart from that mistake, is there anything else that could be unsafe in my code? I assumed that char pointers would be better for user input than char arrays (overflow can cause segfault), just wondering whether it is a good practice.

Joshun

Sorry rags_to_riches, you posted before I refreshed. The analyse function does require access to more than one char when more complicated command words are added later. I've managed to sort that out and changed analyse to

Code:

---

void analyse(char *input)

---

Const char * doesn't work though, it doesn't seem to allow fgets to access the pointer.

I have however changed

Code:

---

gets(input);

---

to

Code:

---

fgets(input, INPUTLEN, stdin);

---

, which works as expected.

You should check the return value of both fgets and malloc, and handle the case where they return null pointers appropriately. Also, it isn't necessary to multiply INPUT_LEN by sizeof (char), because sizeof (char) is guaranteed to be 1.

Although it's not such a huge issue, you ought to put the '\n' character at the end of a line rather than the beginning. Functions like puts work this way, and there's an issue with some C implementations that require text streams to have a terminating newline character on their last line.

Thanks Barney McGrew for the advice. The program now exits if there is not sufficient memory (though incredibly unlikely) and I have removed sizeof(char) since it is needed. I'm not too worried about the \n issue as the C implementations I recommend work fine with it - the makefile requires gcc anyway. The current code now:
https://github.com/Joshun/adventure

improved adventure.c:

Code:

---


// Text Adventure v1 by Joshua O'Leary


// Preprocessor Directives
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "definitions.h"


// Debug Mode when set to 1
#define DEBUG 1


#define INPUTLEN 20


#define PROMPT "\nWhat now? "


int main()
{   
    //char *input = malloc(INPUTLEN * sizeof(char));
    char *input = malloc(INPUTLEN);
    if ( input == NULL ) { printf("Insufficient memory. Exiting...\n"); return 1; }
    printf("\nAdventure v1");
    printf("\nYou are outside the house");
    while ( status.gameover == 0 )
    {
        prompt(input);
    }
    free(input);
    return 0;
}


void control(char value)
{
    switch (value)
    {
        case 'q':
        printf("\nExiting...\n");
        status.gameover = 1;
        break;
    }


}


void prompt(char *input)
{   
    printf("%s", PROMPT);
    //gets(input);
    fgets(input, INPUTLEN, stdin);
    //printf("%c", *input);
    //input++;
    //printf("%c", *input);
    analyse(input);
}


void analyse(const char *input) /* char * for single char */
{
    #if DEBUG == 1
        printf("\nInput: %s", input);
        printf("\nRoom: %d", status.room);
    #endif
    switch ( *input )
    {
        case 'Q':
        case 'q':
            printf("\nquit called");
            control('q');
            break;
        case 'N':
        case 'n':
            gotoroom(0);
            break;
        case 'S':
        case 's':
            gotoroom(1);
            break;
        case 'E':
        case 'e':
            gotoroom(2);
            break;
        case 'W':
        case 'w':
            gotoroom(3);
            break;
        default :
            printf("\n?!");
            break;
    }
}

---

For your program to behave as all standard unix utilities, always print a '\n' at the end of the string (not at the beginning)

Code:

---

/* ... */
    printf("\nAdventure v1");
    printf("\nYou are outside the house");
/* ... */
        printf("\nExiting...\n");
/* ... */
            printf("\nquit called");
/* ... */

---

Code:

---

/* ... */
    printf("Adventure v1\n");
    printf("You are outside the house\n");
/* ... */
        printf("Exiting...\n\n");
/* ... */
            printf("quit called\n");
/* ... */

---

Ok thanks, I have finally sorted out the issues with the \n's. I have also added a function to interpret a 'take' command, which copies the first 5 characters after the word 'take ' from the prompt variable. The improved code:

Code:

---


// Text Adventure v1 by Joshua O'Leary


// Preprocessor Directives
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "definitions.h"


// Debug Mode when set to 1
#define DEBUG 1


#define INPUTLEN 20


#define PROMPT "What now? "


int main()
{   
    //char *input = malloc(INPUTLEN * sizeof(char));
    char *input = malloc(INPUTLEN); if ( input == NULL ) { printf("Insufficient memory. Exiting...\n"); return 1; }
    printf("Adventure v1\n");
    printf("You are outside the house\n");
    while ( status.gameover == 0 )
    {
        prompt(input);
    }
    free(input);
    return 0;
}


void control(char value)
{
    switch (value)
    {
        case 'q':
        printf("Exiting...\n");
        status.gameover = 1;
        break;
    }


}


void buffcheck(char *buffer)
{
    if ( buffer == NULL ) { printf ("Warning: Out of memory!\n"); }
}


void prompt(char *input)
{   
    printf("%s", PROMPT);
    //gets(input);
    fgets(input, INPUTLEN, stdin);
    //printf("%c", *input);
    //input++;
    //printf("%c", *input);
    analyse(input);
}


void analyse(const char *input) /* char * for single char */
{
    char *buffer = malloc(4); buffcheck(buffer);
    #if DEBUG == 1
        printf("Input: %s\n", input);
        printf("Room: %d\n", status.room);
    #endif
    switch ( *input )
    {
        case 'Q':
        case 'q':
            //printf("quit called\n");
            control('q');
            break;
        case 'N':
        case 'n':
            gotoroom(0);
            break;
        case 'S':
        case 's':
            gotoroom(1);
            break;
        case 'E':
        case 'e':
            gotoroom(2);
            break;
        case 'W':
        case 'w':
            gotoroom(3);
            break;
        case 'T':
        case 't':       
            strncpy(buffer, input, 4);
            //printf("\ntake%d", strcmp(buffer, "take"));
            if ( strcmp(buffer, "take") == 0 ) { take(input); free(buffer); break; };
        default :
            printf("?!\n");
            break;
    }
}


void take(const char *input)
{
    char *buffer = malloc(5); buffcheck(buffer);
    strncat(buffer, (input + 5), 5); /* Append input onto buffer, skipped 5 characters */
    printf("Item Requested:%s\n", buffer);
    free(buffer);
}

---