ATM Machine Simulator

Question

I've been playing around in C for a few weeks and decided to write a program that somewhat simulates an ATM machine. I'll give you the code at the end of the post in case you're not interested in dealing with the question at all.

Some facts: There are 2 languages in the program, the 2nd is Slovak. The code for it is the same as the English version, so you can ignore it if you want.

The program isn't perfect. It only deals with integers and usually any user input outside of the pre-defined format makes it do strange things. What I wanted to accomplish is user's ability to at least perform simple operations without the program exiting every time (hence several while (1) loops within the program).

What I'm looking for: I'd like your input on my coding itself. I have a feeling that my code is very cumbersome, bulky, and probably repetitive. Is there anything you'd like to point out? Correct? Is the whole thing completely worthless? Any ideas for improvement?

Reason for asking: I have no previous experience with programming and don't want to get into bad habits right away. Your input is really appreciated.

Anyway, here's the code.

#include <stdio.h>
#include <stdlib.h>
/*ATM machine simulation program*/

void main (){

int lang, pin, operation, amount, i;
int userpin [50];
    userpin [0]= 1234;
    userpin [1]= 9999;
int userbal [50];
    userbal[0] = 1000;
    userbal[1] = 1000;
char user[50];  //used 50 just for buffer purposes. User ID's would be limited to 50 characters 

    while (1){
    system ("cls");
    printf ("\tWELCOME TO THE C-BANK ATM MACHINE PROGRAM!\n\n");
    printf ("Please select 1 for English.\nStlacte 2 pre Slovencinu.\n");
    scanf ("%d", &lang);

    if (lang == 1){

        while (1) {

            system ("cls");
            printf ("Welcome! Please, enter your user ID and press enter\n");
            scanf ("%s", user);
                if (strcmp(user, "Lukas") == 0){
                    system ("cls");
                    printf ("Hi Lukas. Please, enter your 4-digit PIN\n");
                    scanf ("%int", &pin);
                        if (pin== userpin[0]){
                            while (1){  
                                system ("cls");
                                printf("Welcome to your account Lukas! Select one of the following operations and press enter or enter 0 to exit:\n\n\n");
                                printf("1.Check balance\t\t\t2. Widthdraw money\t\n");
                                scanf ("%int", &operation);
                                    switch (operation) {
                                        case 1:
                                        system ("cls");
                                        printf ("Your current balance is $%d\n", userbal[0]);
                                        system ("pause > nul");
                                        break;
                                        case 2:
                                        system ("cls");
                                        printf ("Your current balance is $%d. Please, enter the amount you'd like to withdraw and press enter\n", userbal[0]);
                                        scanf ("%int", &amount);
                                            if (amount <= userbal[0]) {
                                                system ("cls");
                                                userbal[0]= userbal[0] - amount;
                                                printf ("You withdrew $%d. Your remaining balance is $%d", amount, userbal[0]);
                                                system ("pause > nul");
                                            }
                                            else {
                                                printf ("Insufficient available funds.\n");
                                                system ("pause > nul");
                                                }
                                        break;
                                        case 0:
                                        exit (0);
                                        break;
                                        default:
                                        printf ("Invalid entry\n");
                                        system ("pause > nul");
                                        }
                            }
                        }
                        else {
                            printf ("Incorrect PIN\n");
                            system ("pause > nul");
                            fflush (stdin);
                        }
                }
                else if (strcmp(user, "Adam") == 0) {
                    system ("cls");
                    printf ("Hi Adam. Please, enter your 4-digit PIN\n");
                    scanf ("%int", &pin);
                        if (pin== userpin[1]){
                            while (1) {
                                system ("cls");
                                printf("Welcome to your account Adam! Select one of the following operations and press enter or enter 0 to exit:\n\n\n");
                                printf("1.Check balance\t\t\t2. Widthdraw money\t\n");
                                scanf ("%int", &operation);
                                    switch (operation) {
                                        case 1:
                                        system ("cls");
                                        printf ("Your current balance is $%d\n", userbal[1]);
                                        system ("pause > nul");
                                        break;
                                        case 2:
                                        system ("cls");
                                        printf ("Your current balance is $%d. Please, enter the amount you'd like to withdraw and press enter\n", userbal[1]);
                                        scanf ("%int", &amount);
                                            if (amount <= userbal[1]) {
                                                system ("cls");
                                                userbal[1]= userbal[1] - amount;
                                                printf ("You withdrew $%d. Your remaining balance is $%d", amount, userbal[1]);
                                                system ("pause > nul");
                                            }
                                            else {
                                                printf ("Insufficient available funds.\n");
                                                system ("pause > nul");
                                            }
                                        break;
                                        case 0:
                                        exit (0);
                                        break;
                                        default:
                                        printf ("Invalid entry\n");
                                        system ("pause > nul");
                                        break;
                                    }
                            }
                        }       

                    else {
                        printf ("Incorrect PIN\n");
                        system ("pause > nul");
                        fflush (stdin);
                    }
                }
                else {
                    printf ("Incorrect user ID\n");
                    system ("pause > nul");
                    fflush (stdin);
                };
        };
    }
        else if (lang == 2) {
            while (1) {
                system ("cls");
                printf ("Vitajte! Prosim, zadajte svoje uzivatelske meno a stlacte enter.\n");
                scanf ("%s", user);
                    if (strcmp(user, "Lukas") == 0){
                        system ("cls");
                        printf ("Ahoj Lukas. Prosim, zadaj svoj 4-miestny PIN\n");
                        scanf ("%int", &pin);
                            if (pin== userpin[0]){
                                while (1) {
                                    system ("cls");
                                    printf("Vitaj Lukas. Prosim, vyber si jednu z nasledujucich operacii a stlac enter alebo zadaj 0 na ukoncenie programu:\n\n\n");
                                    printf("1.Skontrolovat stav\t\t\t2. Vyber hotovosti\t\n");
                                    scanf ("%int", &operation);
                                        switch (operation) {
                                            case 1:
                                            system ("cls");
                                            printf ("Vas stav na ucte je $%d\n", userbal[0]);
                                            system ("pause > nul");
                                            break;
                                            case 2:
                                            system ("cls");
                                            printf ("Vas stav na ucte je $%d. Prosim, zadajte sumu, ktoru chcete vybrat a stlacte enter\n", userbal[0]);
                                            scanf ("%int", &amount);
                                                if (amount <= userbal[0]) {
                                                    system ("cls");
                                                    userbal[0]= userbal[0] - amount;
                                                    printf ("Vybrali ste $%d. Vas zostatok je $%d", amount, userbal[0]);
                                                    system ("pause > nul");
                                                }
                                                else {
                                                    printf ("Nedostatocny stav na ucte!\n");
                                                    system ("pause > nul");
                                                }
                                            break;
                                            case 0:
                                            exit (0);
                                            default:
                                            printf ("Neznamy prikaz\n");
                                            system ("pause > nul");
                                            break;
                                        }
                                }   
                            }
                            else {
                                printf ("Nespravny PIN\n");
                                system ("pause > nul");
                                fflush (stdin);
                            }
                    }
                    else if (strcmp(user, "Adam") == 0) {
                        system ("cls");
                        printf ("Ahoj Adam. Prosim, zadaj svoj 4-miestny PIN\n");
                        scanf ("%int", &pin);
                            if (pin== userpin[1]){
                                while (1) {
                                    system ("cls");
                                    printf("Vitaj Adam. Prosim, vyber si jednu z nasledujucich operacii a stlac enter alebo zadaj 0 pre ukoncenie programu:\n\n\n");
                                    printf("1.Skontrolovat stav\t\t\t2. Vyber hotovosti\t\n");
                                    scanf ("%int", &operation);
                                        switch (operation) {
                                            case 1:
                                            system ("cls");
                                            printf ("Vas stav na ucte je $%d\n", userbal[1]);
                                            system ("pause > nul");
                                            break;
                                            case 2:
                                            system ("cls");
                                            printf ("Vas stav na ucte je $%d. Prosim, zadajte sumu, ktoru chcete vybrat a stlacte enter\n", userbal[1]);
                                            scanf ("%int", &amount);
                                                if (amount <= userbal[1]) {
                                                    system ("cls");
                                                    userbal[1]= userbal[1] - amount;
                                                    printf ("Vybrali ste $%d. Vas zostatok je $%d", amount, userbal[1]);
                                                    system ("pause > nul");
                                                }
                                                else {
                                                    printf ("Nedostatocny stav na ucte!\n");
                                                    system ("pause > nul");
                                                }
                                            break;
                                            case 0:
                                            exit (0);
                                            default:
                                            printf ("Neznamy prikaz\n");
                                            system ("pause > nul");
                                            break;
                                        }
                                }   
                            }
                            else {
                                printf ("Nespravny PIN\n");
                                system ("pause > nul");
                                fflush (stdin);
                            }
                    }
                    else {
                        printf ("Nespravne uzivatelske meno.\n");
                        system ("pause > nul");
                        fflush (stdin);
                    };

            };
        }

        else {
            system ("cls");
            printf ("Invalid command. Enter any character to return to the home screen\n");
            system ("pause > nul");
        };
    };

getch();

}

Answer 1

The problem here is the lack of functions or subroutines to avoid repeating code, thus making the code less mantainable.

There are here other issues which are also important, for example, multilanguage support, which I'll address in the following

To carry out multilanguage, I would declare an n x m array to store the n strings in m languages. To provide an example of what I mean, to store the greeting message, an array like the following needs to be declared

char multilanguageArray[1][2] = {
    {"WELCOME TO THE C-BANK ATM MACHINE PROGRAM!"},
    {"Vitajte! Prosim, zadajte svoje uzivatelske meno a stlacte enter."}
}

To explain this array, the first dimension of the array declares the amount of messages stored, while the second dimension declares the amount of languages one can output the information.

So, if we would like to print in any of the languages, the following line would just do that.

scanf ("%d", &lang);
printf ("\t%s\n\n", multilanguageArray[1][lang - 1]);

This code would make the greeting valid for both languages, while also making the code more mantainable if you want to change the greeting message for any language/s by having all the messages in the same place.

Keep in mind I substract 1 from lang since arrays are zero-index based.

Implementing language this way can save you a lot of code, make it more readable and also more mantainable since you don't have to look in n lines to fix an error that has propagated through all your languages due to copy pasting code.

This can be applied too to handle any amount of users, further reducing your code and making it, again, more manageable when maintenance or improvements happen (which you are bound to do).

If you need help in developing anything, comment my post with what you need and I'll try to give you an answer as clear as I can. Hope this helped explain how to handle multilanguage.

Note: To ease the maintenance of interface messages, you can switch the matrix, n for languages, m for messages, that way you will have in the same first dimension the same message for all the languages, rather than having all the messages for a language in the first dimension.

Answer 2

There are a number of things you could do to improve your code. Here are some of them:

Consider separating I/O from the algorithm

Right now, everything is done in main. Better practice is to separate things into functions. In particular, I'd recommend separating the input/output routines from the program logic where the code adds and subtracts values from an account.

Use the appropriate #includes

The code uses strcmp and getch which are in <string.h> and <curses.h> (on non-Windows machines). In order to compile and link on my machine, this code requires the following two lines:

#include <string.h>
#include <curses.h>

Isolate platform-specific code

It's unlikely that you really need getch in your program, since it appears to simply be a delay before exiting the program. For that reason, you could use the standard getchar rather than the non-standard getch.

Don't use system("cls")

There are two reasons not to use system("cls") or system("pause"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named cls or pause, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into a seperate functions cls() and pause() and then modify your code to call those functions instead of system. Then rewrite the contents of those functions to do what you want using C. For example, if your terminal supports ANSI Escape sequences, you could use this:

void cls()
{
    printf("\x1b[2J");
}  

Follow standard declaration of main

Your compiler may allow you to declare void main() but that construction is not valid according to the standard. For that reason, you should use the standard int main() instead. Note that you don't need to explicitly provide a return 0; at the end of main -- it's created implicitly by the compiler.

Eliminate unused variables

Unused variables are a sign of poor quality code, and you don't want to write poor quality code. In this code, i is unused. Your compiler is smart enough to tell you about this if you ask

Avoid buffer overrun vulnerabilities

The code currently includes these two statements:

char user[50];
scanf("%s", user);

This is a potential problem because the scanf will read any size of string, but we've only allocated 50 bytes. That's the recipe for a buffer overflow vulnerability and must be eliminated. Fortunately, it's simple to do so:

scanf("%50", user);

Fix your scanf format strings

Several places in the code have a line like this: scanf ("%int", &operation);

The format string "%int" is certainly not what you actually want there. It should probably instead be scanf ("%d", &operation);

Check return values for errors

The call to scanf can fail. You can check the return values to make sure they haven't or your program may crash (or worse) when given malformed input or due to low system resources. Rigorous error handling is the difference between mostly working versus bug-free software. You should strive for the latter.

Think carefully about signed versus unsigned

I can become very wealthy using this ATM by simply withdrawing a large negative number.

Consider consolidating the strings

If you ever need to provide a different language for the interface, it's convenient to have all of strings extracted into a list and named. For example:

static const char BAD_PIN[] = "Incorrect PIN";
static const char BAD_ID[]  = "Incorrect user ID";

Since your program already supports two languages, you could use strings like this:

#define LANG_COUNT 2
static const char *BAD_PIN[LANG_COUNT] = {
    "Incorrect PIN",  // English
    "Nespravny PIN"   // Slovencinu
};

Then when you need to print a string, it could look like this

puts(BAD_PIN[lang]);

Where lang would be 0 for English and 1 for Slovak. Even better, use a macro for this:

#define TSTR( name, eng, slo ) static const char *name[LANG_COUNT] = { eng, slo }

Then use it like this:

TSTR(BADPIN, "Incorrect PIN", "Nespravny PIN");

Use a data stucture to organize the code

Each account has a user name, pin, and account balance. Your program would be much nicer and easier to maintain if you defined a structure such as this:

struct account {
    char name[50];
    char userpin[4];
    unsigned balance;
};

Now all of the operations can be done on an account structure and all the program needs to maintain is an array of accounts.

Avoid while(1)

There are very many places in the code that now have while(1) but the code doesn't really continue forever. Consider instead defining an actual exit variable such as bool done; that gets set and then checked. This makes it easier for someone reading the code to figure out where and under what conditions a loop actually ends.

Consider security

It's not too soon to think about security as you program. In particular, the account names and PINs are all stored within the program as cleartext. In a real ATM, it would be better not to store them that way because they would easily be extracted by someone with access to either the source code or the binary executable. One way to handle that would be to encrypt the messages, or to instead store and use a cryptographic hash instead of the name and PIN. I realize this is just a toy program right now, but it's good to get into the habit of thinking about secure programming from the beginning.

A simpler thing that could easily be done now is to ask for the name and PIN together and then simply reject unless both match without saying which part was bad. Otherwise, an attacker could continue to guess names until he got a "Bad PIN" prompt at which point he would know that the account name was valid. This is caused information leakage and should be avoided.

Answer 3

Why repeat the code for the two languages? Updates and improvement are harder to do on repeated code.

For example you may write a function

bilingual_print(eng, other)

That will read the global language flag and print accordingly. This will cut down your code by half.

The function may be defined as:

 void bilingual_print(const char* a, const char b) {
     puts(lang == 1 ? a : b);
 }

You may then delete the if lang == 1 and else branch.

---

As @Edward correctly noted, global variables are best avoided, so the function should really be:

 void bilingual_print(int lang, const char* a, const char b) {
     puts(lang == 1 ? a : b);
 }

So that the functionality is more self-contained and easier to grasp.