Keeping bank account records using a struct

Can you just shoot me some ideas on how to better structure the program and also if you notice any big no-nos at first glance? This is certainly not production code, it is my first attempt at a data structure implementation in C, which I am also new to. One thing I am aware of/will structure differently next time is the .h files. When I started this program, I hadn't realized that essentially, .h files is how you "encapsulate" or information-hide, and I had been trying to just get practice linking .h files into the project. The only issue I had is that I'd like to move the struct def out of main.h but some other source files need the struct def. What's the best way to handle this? Should I have made the functions take a pointer to the struct?

Main.h:

#ifndef MAIN_H_INCLUDED
#define MAIN_H_INCLUDED

void clearInput();
void addNewAccount();
char* myGets();
void listAll();
int promptForDelete();
void loadRecordsFromFile();
void saveRecordsToFile();
void promptAndStoreRecordInMemory();

struct account {
    int number;

    char lastname[15];
    char firstname[15];

    float balance;
    struct account *next; /*This is the "Link" to the next list record/node*/
};

struct account *firsta, *currenta, *newa;

#endif // MAIN_H_INCLUDED

Main.c:

```
#include
#include
#include
#include
#include "deleteARecord.h"
#include "modifyARecord.h"
#include "main.h"
int anum = 0;

/PROGRAM ENTRY POINT/
int main() {
char ch;
firsta = NULL;
loadRecordsFromFile();

do {
clearInput();
puts("A - Add a new account");
puts("D - Delete a record");
puts("M - Modify a Record Account #");
puts("L - List all records");
puts("Q - Save Records and Quit this program\n");
printf("\tYour choice:");
ch = getchar();
clearInput();
ch = toupper(ch);
switch (ch) {
case 'A':
puts("Add new account\n");
printf("Original Input: %c", ch);
addNewAccount

Buffer Safety

You're using a fixed buffer size for your names(15 bytes), however you're using gets to read the input directly into the fields. If the user enters more characters than the buffer can handle it will result in a buffer overrun, trashing whatever happens to be in the memory afterwards.

Header Files / modules

Rather than having a header file for each operation, I would tend to have a header for the section of functionality. Possibly 'accounts.h' would be appropriate. This header would define all of the relevant interfaces (add,modify,delete) for your structure. Personally, I'd also put the implementation for these methods in the same '.c' file, rather than distributing them across different files. They're performing actions on the same data structure, so it feels like they belong together. As it stands, it is weird that add is in main.c and the rest of the functions are in different files.

switch/break/return

You've got a switch statement:

switch(accountNumbersPtr->newNumber)
    {
    case 0:
        accountNumbersPtr->success = FALSE;
        return accountNumbersPtr;
        break; //Not reachable but for semantics
    default:
        accountNumbersPtr->success = TRUE;
        return accountNumbersPtr;
        break; //Not reachable but for semantics
    }

Don't do this (it adds nothing but noise which is distracting):

break; //Not reachable but for semantics

Also, if you've only got two cases as in this case it should be an if statement, not a switch.

ID

On the surface of it, I'd expect the ID to be unique, however you don't ensure this. It's possible to get multiple records with the same ID. Is this by design? Because it feels like a bug.

malloc

You don't need to cast the return from malloc. Sometimes you don, sometimes you don't. Be consistent, it makes your code easier to read and generally err on the side of not introducing unnecessary code (so don't do the cast) like this:

newa = (struct account *)malloc(sizeof(struct account));

Memory leak?

This looks like a memory leak:

newa = (struct account *)malloc(sizeof(struct account));
/*This line reads each struct from the file and stores in currenta*/
fread(currenta,sizeof(struct account),1,f);
/*If we're at the last record, break out of the loop.*/
if(currenta->next == NULL)
    break;

When reading from the file, you create a new node and keep reading until you get to the last node, however you're allocating memory ahead of time. This means that when you read the last node from the file, newa is pointing some newly allocated memory that you've not put anywhere else. If you then add a new record, the pointer is overwritten and the memory is lost. This is perhaps a symptom of using global variables, which are much harder to keep track of than locals. Does newa really need to be global, rather than a local variable?

static

Where you're not going to be calling methods / referring to global variables from another source file, consider making them static so that they're confined to that source file (you would then not include them in your header file). So, for example I wouldn't have loadRecordsFromFile in the 'main.h' file. It's not being called from any other sources so there's no reason to export it. I'd instead have the function prototype at the top of the 'main.c' file.

StackExchange Code Review Q#149387, answer score: 2