Thread: Help almost there!

Fair warning: I am reasonably drunk and this is too much stuff to proofread so sorry if a lot of this is wrong and riddled with spelling/grammatical errors.

In future, you should try to develop iteratively. This means start by creating the smallest working program that accomplishes part of your goal and then build on top of that.

We could be here a while...

Code:

#include <conio.h>

conio.h is not a part of standard C. For comparison's sake, something like stdio.h is part of standard C. Being a part of standard C means that all of the interfaces are well defined and the functionality 'provided' by that header will be available wherever the compiler is.
Not being a part of standard C is not necessarily a bad thing. windows.h for example is windows specific and provides prototypes for lots of useful windows-specific functions. Unfortunately though, if you aren't a part of standard C, you have no guarantee of your functions still being supported in 20 years time.
In fact this is exactly what has happened to conio.h. It is no longer supported by the majority of compilers. This is not really a bad thing though because to be completely honest, you really do not need it.

Code:

#include <windows.h>

Using windows-specific functions is fine as it helps to extend the functionality of your application. In your case though, it is not essential. All it is doing here is adding a complexity that is disproportionally large compared to the utility that it adds to your program. I'd get rid of it (and any function calls that use it) at least until the logic of you application works first. Function before form.

Code:

SetConsoleTextAttribute(GetStdHandle(STD_OUTPUT_HANDLE), 10);

This is what I said above, so I won't harp on. That said, if you have to use Windows-specific functions then I don't understand why you're using numeric constants instead of their appropriate preprocessor defined constants (e.g you're using 10 here instead of FOREGROUND_BLUE or whatever 10 means I can't be bothered looking it up). It makes your code much harder to understand.

Function prototypes are good. Generally speaking, many functions that return void and take 0 arguments are indicative of bad design though.

Your structure to hold information about accounts is good. Sometimes many fields are indicative of bad design though. A lot of the time this is just because people are involved and so extra fields are needed for security/identification and whatnot so I will give you the benefit of the doubt here. If any of your fields can be computed solely from other fields though I would consider removing those fields.

Your use of global variables is... sorry to say but horrid. It may not bother you because it's for an assignment or something, but it kills the people trying to help you. Let's say that your assignment now changed from having to manage one account to having to manage.... (god forbid) TWO ACCOUNTS! How would you possibly account for that? How about arbitrarily many accounts? Using local variables properly not only allows for this but makes your code much easier to read and debug. Those functions that return void and take no arguments? They should probably take an Accounts structure and possibly a file structure depending on context and return a success/failure code.

Your indentation needs a lot of work. I like to think that good indentation indicates a good understanding of the programming constructs that you're using. This is somewhat like how good sentence structure indicates clarity of thought in the writer (something I'm working on ).

Code:

int pass=0;
   ...

                 printf("Enter password ");
                 scanf("%d", &pass);

Why are you initialising pass when your first act is to read it in from the user? If you're initialising it, I would think that you were going to use it as an iterative variable or something.

Code:

int program_pass=2014;

If this isn't going to change during the execution of your program, consider making it const (const int program_pass = 2014 to let us (and your compiler) know.

Code:

 do  //CODES FOR ENTERING THE PASSWORD
     {
    ...
     }
                while (pass!=program_pass);
 
 
                while (pass==program_pass)  // while loop for when password entered is correct
                {
                    printf(" \nAccess Granted!\n");
                    printf("\n\nPress enter to continue");
                            getch();
                            system("cls");
                            display_main_menu();
                }

The test expression of your while loop does not really make a lot of sense. If you're at that point of the program then the entered password is definitely equal to your 'program_pass' variable. It looks to me like there is no way of breaking out of this loop and properly exiting the program. Consider something like:

Code:

do
{
    printf("Access granted\n");
    /* Other code here */
    printf("Would you like to continue? (Y/n) ");
    scanf("%c", userChoice );
} while( userChoice == 'Y' );

Code:

main_menu:        // goto LABEL FOR MAIN MENU

Nope. Nope....Nope.nope.nopenopenope nope nope.
Like everything, labels have their place. Unfortuntaly even I'm not smart enough to use them properly. Stick to structured programing for now. We can can use a loop here instead.

Code:

scanf("%s",&activity_selection);

The %s format specifier will read in a sequence of characters. You do not want a sequence of characters, you only want a single character. %c will do the trick here.

It's incredibly confusing to see which statements belong to the default statement of your switch given your current 'style' of indentation. I don't understand why you're asking for input again in your default case though. Why not just loop back to the start of your function and get the user to enter their input again in much the same fashion as you do in your main function?

Code:

int r;

What does this variable do? r is generally accepted to be representative of the radius of a circle so I'm going to assume that is what it is here. Strange variable to have in a 'create_account' function though.

Code:

srand((unsigned)time(0));//UNSIGNED PREVENTS RANDOM NEGATIVE NUMBERS FROM GENERATING

No it doesn't.

Code:

r=(rand()*999999999);
r=(rand()%9999999999999)+1;

This will possibly yield negatives sometimes as that number to the right is large and might cause integer overflow. Not sure why you're multiplying it by anything in the first place. The number generated by rand() is guaranteed to be between 0 and RAND_MAX.

Code:

decision='y';

Not sure why this line of code exists.

Code:

scanf("%s",&payment_plan);

Again, %c.
And again, indent... wow this is painful.

Code:

case 'A': case'a':
    printf("You have decided to use a savings plan of 1 year\n");
    students.payment_type=1;
    break;
 
case 'B': case'b':
    printf("You have decided to use a savings plan of 2 years\n");
    students.payment_type=2;
    break;
 
case 'C': case'c':
    printf("You have decided to use a savings plan of 3 years\n");
    students.payment_type=3;
    break;
 
 
case 'D': case'd':
    printf("You have decided to use a savings plan of 4 years\n");
    students.payment_type=4;
    break;
 
 
case 'E': case'e':
    printf("You have decided to use a savings plan of 5 years\n");
    students.payment_type=5;
    break;

Is a good first attempt but can be condensed. e.g

Code:

students.payment_type = payment_plan - 'A';
printf("You have decided to use a savings plan of %d years\n", students.payment_type );

You just need to check that the payment_plan is between A and E (and account for lowercase). The code you have works fine though so if you are concerned that you might make a mistake then just leave it.

Code:

if(students.acc_fee<100)
{
    ...
}
else if(students.acc_fee<=100)
{
    ...
}

These expressions are tested and acted upon in order. If students.acc_fee satisfies the first comparison then none of the later expressions will be tested. The second expression should be if( students.acc_fee == 100 ).

Code:

fprintf(acc,"%d",students.account_num);
    fprintf(acc,"%s",students.firstname);
    fprintf(acc,"%s",students.lastname);
    fprintf(acc,"%s",students.parish);
    fprintf(acc,"%d",students.yob);
    fprintf(acc,"%d",students.age_calculation);
    fprintf(acc,"%d",students.payment_type);
    fprintf(acc,"%s",students.password);
    fprintf(acc,"%.2f",students.acc_bal);

I'd suggest outputting a newline after all of these to make sure that they read in properly. For example fprintf( acc, "%s\n", students.firstname );

Code:

scanf("%s",&decision);

%c

Code:

do
{
    ....
}
while(decision=='Y'||decision=='y');
if(decision=='N'||decision=='n')
{
...
}

Not sure why you've decided to only run that code when decision is N or n. If you have exited the loop then decision must not be Y or y.

Code:

main_or_exit:

NOOOOOOOOOOOOOOOOOOOPE

Code:

    printf("\n\t      ||                A.<MAIN MENU>            ||\n\t");
    printf("      ||                B.<EXIT>                 ||\n\n");
    printf("Please select the desired option <a or b>\n\n");

Why even have this option here? Why not return and let the user decide what they want to do?

Code:

             rewind(acc);
             fseek(acc,(id -1) *sizeof (Accounts), SEEK_SET);
             fread(&students, sizeof(Accounts), 1, acc);

This code won't work with the way you're writing to the file. You should read in each field individually. As for skipping to the required account you can read until you have read the required number of newlines and then start reading from that point.

There are a few other small errors but to be completely honest the biggest problems with your program are structural rather than logical. There is so much duplicate code that doesn't really need to be there. These things are all learned with time but you need to be willing to refactor your code when you find a better approach. If you are scared to refactor then learn to make backups.

What I think you should do:

Code:

int main( void )
{
    char choice;
    Account accData;

    do
    {
        printf("C -- Create account\nD -- Delete account\nU -- Update account\n" );
        printf("E -- Exit");
        scanf( "%c", &choice );

        switch( choice )
        {
            case 'C':
                createAccount( &accData );
                break;
            case 'D':
                ...
            case 'U':
                ...
            case 'E'
                break;
            default:
                printf( "Unrecognised option. Please try again!\n");
                break;
        }
    } while( choice != 'E' );

    return 0;
}

You may think "well, all these things you are telling me aren't essential for me to get my code working". That isn't entirely incorrect. I try my best to make my comments as simple as possibly to implement. Once you implement my suggestions I'd be very surprised if you couldn't find the bugs easily by yourself. There is no royal road to programming.

Originally Posted by DeadPlanet

Fair warning: I am reasonably drunk and this is too much stuff to proofread so sorry if a lot of this is wrong and riddled with spelling/grammatical errors.

I know this trick.

Originally Posted by C-learner

read in each field individually? how should i structure that? should i use fread? probably fscanf? which is associated with the way i wrote the data to the file(which is fprintf);

Try to never mix fread/fwrite with fprintf/fscanf, use either one set or the other.

The way you have it now I would recommend using fscanf to read in each field, in a similar (but opposite) way to how you have written the fields to file.

Ok so it now reads from the file after the program is closed and re-opened thanks for the help DeadPlanet i now love fscanf and fprintf and how to use them correctly(which i had no knowledge of b4 i posted here) ... now all that's left is this.

Originally Posted by DeadPlanet

As for skipping to the required account you can read until you have read the required number of newlines and then start reading from that point.

How do i do this? it reads from the file but it can only read and output the first account inside the file and not the others below it. I havent the slightest clue how to skip lines until it reaches the account number im looking for and read from that to the end of the structure info....

Originally Posted by C-learner

How do i do this? it reads from the file but it can only read and output the first account inside the file and not the others below it. I havent the slightest clue how to skip lines until it reaches the account number im looking for and read from that to the end of the structure info....

Assuming each member of your structure is on it's own line of the file then yiu know that each structure takes up x lines (where x is the number of members in your structure). If I want to get to the i-th structure in the file then I know that there are (i-1)*x lines before I am ready to read.
Lines in a file are split up by a special character called the newline character. This character is denoted by '\n' in C. If you read (i-1)*x newline characters then you are now ready to read the i-th structure.

How do i let the program know the i-th structure(the structure with the account number entered for searching) which its gonna skip lines to..ive been reading about fgetpos() and fsetpos()...then again there is fseek() and ftell()....The idea i got on how to do this is search through the file skipping a certain amount of bites using fseek and when i reach the desired account number if it matches then i use fscanf to read the information

I don't really know what you're asking. You can use fseek or fsetpos if you like but it's overcomplicating the issue. Just start at the beginning of the file and read characters (discarding them) until you are at the structure that you want. If you want one with a specific account number then you can simply just read every record until you find the appropriate account number.

Im having a small problem displaying all the accounts on the screen...After using the condition(while!feof(acc)) it puts an extra copy on the screen of the last account being printed
..this is my code:

Code:

void display_all_accounts(void)
{
     if((acc=fopen("Active Accounts.txt","r"))==NULL)
         {
          printf("\nERROR OPENING FILE.");
            printf("Press enter to continue");
            getch();
            action_main_menu();
         }
         printf("THESE ARE THE EXISTING ACCOUNTS\n\n");
         printf("==================================================================\n");
         printf("FIRSTNAME        LASTNAME        PASSWORD       AGE       ACOUNT#\n");
         printf("==================================================================\n");
         do{
             fscanf(acc,"%d" ,&students.account_num);
             fscanf(acc,"%s" ,students.firstname);
             fscanf(acc,"%s" ,students.lastname);
             fscanf(acc,"%s" ,students.parish);
             fscanf(acc,"%d" ,&students.yob);
             fscanf(acc,"%d" ,&students.age_calculation);
             fscanf(acc,"%d" ,&students.payment_type);
             fscanf(acc,"%s" ,students.password);
             fscanf(acc,"%f",&students.acc_bal);
             printf("\n%s          %s           %s    %d     %d\n",students.firstname,students.lastname,students.password,students.age_calculation,students.account_num);  
            }
         while(!feof(acc));
}