9
\$\begingroup\$

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

}
\$\endgroup\$
  • 1
    \$\begingroup\$ First of all split your program into multiple functions!!! After that you may start looking to make it better (for example removing big switch statements and reusing repetitive code). To understand why this is important: pick a random line in the middle of your source code and tell how/when/why it will be executed. Now imagine it's not a toy program but a 500000 lines of code real world program... \$\endgroup\$ – Adriano Repetti Dec 2 '15 at 13:32
  • 14
    \$\begingroup\$ I feel obligated to point out that ATM is short for Automatic Teller Machine. Saying "ATM Machine" is like saying Personal PIN Number. ;) \$\endgroup\$ – Hugo Zink Dec 2 '15 at 15:23
  • 2
    \$\begingroup\$ @HugoZink Actually you're wrong, if you want to argue here. "ATM Machine" would equal "Automatic Teller Machine Machine", whereas "PIN Number" would equal "Personal Identification Number Number", not "Personal PIN Number".. You're analogy would only be correct if someone were to say "Automatic ATM Machine"....get it? \$\endgroup\$ – Evan R. Dec 2 '15 at 15:59
  • \$\begingroup\$ I have to say, both of you have great attention to detail, something I lack in this case :D \$\endgroup\$ – LukasS Dec 3 '15 at 7:49
7
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the input! I'm going to attempt to rewrite the program from scratch. Just to check that I understand this right- all messages will be stored in one large array that will be declared outside of the main () function. Once I have the array created, I will reference each message separately throughout the program from the same array. Or should each message in both languages be stored in a separate array? Thanks again! \$\endgroup\$ – LukasS Dec 3 '15 at 7:55
  • 1
    \$\begingroup\$ They should be stored in the same array, if you keep them un separare arrays it kills its purpose since you will have to distingish in between them. \$\endgroup\$ – Oscar Guillamon Dec 3 '15 at 8:03
  • \$\begingroup\$ I'm not sure if I understand this right. char messages [2][15] -an array of 2 rows and 15 columns. Does that mean I can store 15 messages of any length in 2 languages (considering I assign the first dimension to languages) or do I have to consider the length of the longest string when creating this array? Do I then add a 3rd dimension or will it be char messages [2 languages] [length_of_the_longest_string]. Thanks! \$\endgroup\$ – LukasS Dec 3 '15 at 8:53
  • \$\begingroup\$ If I am not wrong, the first dimension refers to the languages, and the second one refers to the amount of messages available. \$\endgroup\$ – Oscar Guillamon Dec 3 '15 at 9:04
  • 1
    \$\begingroup\$ You are making some good progress, keep it up, for further discusion i would ask you to message me directly \$\endgroup\$ – Oscar Guillamon Dec 3 '15 at 9:51
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thank you so much @Edward!! These are all exactly the inputs I need. Many of the errors that you pointed out above come from the fact that since the beginning I haven't really followed a set of instructions. I just simply got an idea of a program or found one online, did my best to make it work and pieced it together using different sources. That's where the "system" commands come from, the getch() at the end of the program (which I used on the simpler programs to prevent them from closing). I'll try to rewrite the program from scratch and post it here afterwards if it works. Thanks again!! \$\endgroup\$ – LukasS Dec 3 '15 at 8:01
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I'm not sure what you mean. Wouldn't I have to write all the instructions in Slovak anyway?? \$\endgroup\$ – LukasS Dec 2 '15 at 13:38
  • 2
    \$\begingroup\$ I added the beginner tag because I think it's very fitting here. Would you mind updating your answer accordingly? \$\endgroup\$ – Mast Dec 2 '15 at 13:40
  • \$\begingroup\$ @LukasS You will have to repeat the messages, not the logic like you do know \$\endgroup\$ – Caridorc Dec 2 '15 at 13:57
  • 1
    \$\begingroup\$ ..............*now \$\endgroup\$ – Caridorc Dec 2 '15 at 14:10
  • \$\begingroup\$ This answer could be better if it didn't seem to advocate using a global variable lang. \$\endgroup\$ – Edward Dec 2 '15 at 18:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.