Simple ATM program with menu

[Hodor]

Looking at menu_getsel(), is there a better way to do this? I.e. I have to be able either read a character or an integer depending on the menu input type so I suppose I need the two fscanf()s although maybe just the format string could be changed (I'm not sure how though). Also the fscanf(in, "%*s")...

#include <stdio.h>
#include <ctype.h>

struct menuitem {
    const char *text;       /* Text to display */ 
    int key;                /* Value that matches this selection */
    int value;
    int casesensitive;
};

struct menudef {
    const struct menuitem *items;  /* Pointer to array of menu items */
    size_t nitems;          /* Number of items in the array */
    int invalidvalue;       /* Value to return if a selection is invalid */
    int inputerrvalue;      /* Value to return on input (stream) error or eof */
    int numerickeys;        /* If 0, menu keys are char, otherwise int */
    const char *title;      /* Displayed at the top of the menu */
    const char *prompt;     /* Prompt for input */
    const char *errormsg;   /* Error message to display if selection invalid */
    const char *iprologue;  /* String to print before each menu item's key */
    const char *iepilogue;  /* String to print after each menu item's key */
};

enum {
    MENU_CASENOTSENSITIVE = 0,
    MENU_CASESENSITIVE    = 1,
   
    MENU_NUMERICKEYS    = 0,
    MENU_CHARKEYS       = 1,
    
    MENU_REPEATONERROR  = 1
};
    

int menu(const struct menudef *m, int repeat, FILE *in, FILE *out);
static void menu_display(const struct menudef *m, FILE *out);
static int menu_getsel(const struct menudef *m, FILE *in);
static int menu_keyvalue(const struct menudef *m, int key);

int menu(const struct menudef *m, int repeat, FILE *in, FILE *out)
{
    int selection;
    int again;
        
    do {
        menu_display(m, out);
        selection = menu_getsel(m, in);
        if (selection == m->inputerrvalue)
            break;
        again = selection == m->invalidvalue && repeat == MENU_REPEATONERROR;
        if (again && m->errormsg)
            fputs(m->errormsg, out);
    } while (again);
    
    return selection;
}

static void menu_display(const struct menudef *m, FILE *out)
{
    size_t i;
    
    if (m->title)
        fputs(m->title, out);
    for (i = 0; i < m->nitems; i++) {
        if (m->iprologue)
            fputs(m->iprologue, out);
        if (m->numerickeys == MENU_NUMERICKEYS)
            fprintf(out, "%d", m->items[i].key);
        else
            fputc(m->items[i].key, out);
        if (m->iepilogue)
            fputs(m->iepilogue, out);
        if (m->items[i].text)
            fputs(m->items[i].text, out);
        fputs("\n", out);
    }
    if (m->prompt)
        fputs(m->prompt, out);
}

static int menu_getsel(const struct menudef *m, FILE *in)
{
    int n;
    char c;
    int key;
    int ns;
    
    if (m->numerickeys == MENU_NUMERICKEYS) {
        ns = fscanf(in, "%d", &n);
        if (ns != 1)
            fscanf(in, "%*s");
    }
    else
        ns = fscanf(in, " %c", &c);
    
    if (ns != 1) 
        return ferror(in) || feof(in) ? m->inputerrvalue : m->invalidvalue;
    
    key = m->numerickeys == MENU_NUMERICKEYS ? n : c;
    
    return menu_keyvalue(m, key);
}

static int menu_keyvalue(const struct menudef *m, int key)
{
    size_t i;
    
    for (i = 0; i < m->nitems; i++) {
        int search;
        if (m->numerickeys == MENU_CHARKEYS) {
            search = m->items[i].casesensitive == MENU_CASENOTSENSITIVE 
                        ? toupper(key) 
                        : key;
        } else
            search = key;
            
        if (m->items[i].key == search)
            break;
    }
    
    return i == m->nitems ? m->invalidvalue : m->items[i].value;
}

/*********************************************************************/

enum {
    SEL_ERROR = -2,
    SEL_INVALID = -1,
    SEL_ORANGES,
    SEL_APPLES,
    SEL_PEARS
};

int main(void)
{
    static const struct menuitem mitems[] = {
        { "Oranges",    1, SEL_ORANGES,   MENU_CASENOTSENSITIVE },
        { "Apples",     2, SEL_APPLES,    MENU_CASENOTSENSITIVE },
        { "Pears",      3, SEL_PEARS,     MENU_CASENOTSENSITIVE }
    };
    static const struct menudef mainmenu = {
        mitems,
        sizeof mitems / sizeof mitems[0],
        SEL_INVALID,
        SEL_ERROR,
        MENU_NUMERICKEYS,
        "Dispense cash as:\n",
        "Select> ",
        "\nInvalid selection, try again\n",
        "  (",
        ") "
    };
    
    int selection;
    
    puts("\n/\\/\\/\\/\\/\\/\\ Welcome to your ATM /\\/\\/\\/\\/\\\n");
    selection = menu(&mainmenu, 1, stdin, stdout);
    switch (selection)
    {
        case SEL_ORANGES:
        case SEL_APPLES:
        case SEL_PEARS:
            printf("Hodor gives you your %s.\n", mitems[selection].text);
            break;
        default:
            printf("Hodor, Hodor, Hodor!\n");
            break;
    }
    return 0;
}

[phantomotap]

O_o

I can't decide if this is another mimetic hazard or a real question; I very nearly ignored the question--intended only to mock--for that reason.

*shrug*

If you are trying to make a general purpose utility which doesn't own error conditions, you shouldn't make the decision of how to clear the error state from the stream. The client is responsible for handling errors in the example; you should just return the relevant error code without the attempt at clearing the buffer.

if (m->numerickeys == MENU_NUMERICKEYS)
    ns = fscanf(in, "%d", &n);
else
    ns = fscanf(in, " %c", &c);
/* or */
ns = (m->numerickeys == MENU_NUMERICKEYS) ? fscanf(in, "%d", &n) : fscanf(in, " %c", &c);
/* with */
if (ns != 1) 
    return ferror(in) || feof(in) ? m->inputerrvalue : m->invalidvalue;

If you want the implementation to handle error conditions, you should check `feof' and `ferror' before the call to `fscanf'--the second call may incorrectly change the value of `errno' making debugging much harder than it needs to be--and provide a callback so that the client might respond with the option to retry or terminate.

int retry(menudef * a_menudef) {
    if (a_menudef->retry) {
        return a_menudef->retry(a_menudef);
    } else {
        if(--a_menudef->attempts) {
            return true;
        } else {
            return false;
        }
    }
}
/* ... */
int error(menudef * a_menudef) {
    /* the error response */
}
/* ... */
do {
    if (/* ferror(in) || feof(in) */) {
        /* attempt to clear errors */
    }
    if (m->numerickeys == MENU_NUMERICKEYS)
        ns = fscanf(in, "%d", &n);
    else
        ns = (m->numerickeys == MENU_NUMERICKEYS) ? fscanf(in, "%d", &n) : fscanf(in, " %c", &c);
} while(ns != 1 && retry(&a_menudef));

Soma

[Hodor]

Yes, that's true thanks.

I've also decided to return a pointer to the struct menu item, rather than an int (or NULL if invalid).

It's not mocking... I realised the other day it's been years since I've done an "interactive" program of this type and was bored so decided to write something.

Edit: yeah, the retry() function is a good idea as well. I'd already decided to make the input (and output) to be handled by a callback, and this adds to that idea.

Edit: Perhaps "oranges" as cash may be perceived as mocking, but it was meant more as a joke and to indicate that this is not critical code. The code itself is, somewhat, serious though.

[phantomotap]

I've also decided to return a pointer to the struct menu item, rather than an int (or NULL if invalid).

O_o

I would "bake" the entries into an generalized event.

Yes. The code is more difficult to correctly implement. Yes. The interface has slight redundancy.

However, I value the approach for the versatility and more closely resembling the actual user interaction.

int apple(menuitem *, void * user);
int orange(menuitem *, void * user);
int vegetable(menuitem *, void * user);

struct mydata_TAG {/* ... */} mydata;

static const struct items =
{
    {"Apples", 'A', &apple, &mydata, CHARACTER | INSENSITIVE}
  , {"Oranges", 'O', &orange, &mydata, CHARACTER | INSENSITIVE}
  , {"Potato", 1, &vegetable, &mydata, NUMERIC}
  , {"Cabbage", 2, &vegetable, &mydata, NUMERIC}
};

Soma

[Hodor]

Interesting... I'll definitely give that some thought!

[gemera]

Always interesting to look at other programmers code here.

There are a couple of issues with it though.

1) 1 2 3 is a valid input and defaults to the first number, which maybe was your intent anyway.

2) There's a buffer flushing issue if the entry is invalid e.g. 9 - the retry code gets printed twice.

[Hodor]

Always interesting to look at other programmers code here.

There are a couple of issues with it though.

1) 1 2 3 is a valid input and defaults to the first number, which maybe was your intent anyway.

For that case, yeah I intend that for the sake of simplicity... if there is extra stuff in the input stream then the calling function/program will have to deal with that. Related to this is the plan to have a callback function for dealing with the input/output instead of passing two FILE pointers to menu() in which case the input may not be coming from a stream at all, so at the moment I won't worry about it.

2) There's a buffer flushing issue if the entry is invalid e.g. 9 - the retry code gets printed twice.

Hmm. I can't reproduce?

/\/\/\/\/\/\ Welcome to your ATM /\/\/\/\/\

Dispense cash as:
  (1) Oranges
  (2) Apples
  (3) Pears
Select> 9

Invalid selection, try again
Dispense cash as:
  (1) Oranges                                                                   
  (2) Apples                                                                    
  (3) Pears                                                                     
Select>

If I redirect stdin (e.g. from a file) I can't reproduce it either. ?????

[gemera]

Sorry, its with the input: 9 9 etc

[Hodor]

Sorry, its with the input: 9 9 etc

Ah yeah, it would do that. Thanks. I suppose I should "flush" the input stream any time an invalid selection occurs