4
\$\begingroup\$

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 <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <ctype.h>
#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();
            break;
        case 'D':
            puts("Delete a record\n");
            deleteARecord();
            break;
        case 'L':
            puts("List All Accounts\n");
            listAll();
            break;
        case 'M':
            puts("Modify a Record\n");
            modifyARecord();
            break;
        case 'Q':
            puts("Quit\n");
            break;
        case '\n':
            ch = getchar();
        default:
            break;
        }
    } while (ch != 'Q');

    saveRecordsToFile();
    if (newa != NULL){
        free(newa);
        printf("\n Memory freed!");
    }
    return(0);
}

void saveRecordsToFile(){
FILE *f;
currenta = firsta;
if(currenta == NULL)
    return;

    f = fopen("record.tdb","w");
    if(f == NULL)
    {
        printf("Error writing to file!\n");
        return;
    }

    while(currenta != NULL)
    {
        fwrite(currenta,sizeof(struct account),1,f);
        currenta = currenta->next;
    }
    fclose(f);
    printf("Data saved to disk!\n");
    return;

}


void loadRecordsFromFile(){
    FILE *f;
    f = fopen("record.tdb","r");
    if(f == NULL){
        printf("Error opening file or file does not exist yet...");
        return;
    }
    firsta = malloc(sizeof(struct account));
    currenta = firsta;

    while(1){
        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;

        /*Otherwise, continue to the next iteration/link in file*/
        currenta->next = newa;
        currenta = newa;
    }
    fclose(f);
    anum = currenta->number;
}
void clearInput(void)
{
    fflush(stdin);
}

void listAll(void){

    unsigned int counter = 0;
    if(firsta != NULL)
        currenta = firsta;
    else{
        printf("There are no items in the list.\n");
        return;
    }
    while(currenta != NULL){
        printf("----------------------------\n");
        printf("ID#:%d\n",currenta->number);
        printf("Last Name: %s\n",currenta->lastname);
        printf("First Name: %s\n", currenta->firstname);
        printf("Balance: %.2f\n", currenta->balance);
        counter++;
        currenta = currenta->next;
    }

}

//////////////////////////////////////////////////////////////
void addNewAccount(void) {
    newa = malloc(sizeof(struct account));

    /*If this is the first record, initialize all pointers to this record*/

    if (firsta == NULL)
        firsta = currenta = newa;
    /*Otherwise, find the end (NULL ptr) and insert there*/
    else {
        currenta = firsta; /*Select first record then loop thru all records til end*/

        while (currenta->next != NULL)
            currenta = currenta->next;
        /*Last record found*/
        currenta->next = newa; /*assign the last record's NEXT ptr to our new record to be inserted*/
        currenta = newa; /*switch to the newly inserted record and make it currently selected*/
    }

    /*Fill in the new record with data*/
    anum++;
    promptAndStoreRecordInMemory();

}
//////////////////////////////////////////////////////////////
void promptAndStoreRecordInMemory()
{
    printf("%27s: %5i\n", "Account number", anum);
    currenta->number = anum;
    clearInput();
    printf("%27s: ", "Enter customer's last name");
    gets(currenta->lastname);
    //getchar(); //Clear buffer
    printf("The lastname is: \"%s\"", currenta->lastname);
    printf("\n");
    printf("%27s ", "Enter customer's first name");
    gets(currenta->firstname);
    //getchar(); //Clear buffer
    printf("\n");
    printf("%27s: $", "Enter account balance");
    scanf("%f", &currenta->balance);
    currenta->next = NULL;
    return;
}
//////////////////////////////////////////////////////////////
int promptForDelete(void){
    int numberToDelete;
    char shouldContinue;
do{
    printf("Enter an ID number of the record you want to delete: ");
    scanf("%d",&numberToDelete);
    printf("You entered %d, is this correct? (y/n)",numberToDelete);
    getchar();
    shouldContinue = getchar();
    shouldContinue = toupper(shouldContinue);
    /*TODO: Refactor*/
    switch(shouldContinue){
        case 'Y':
        return numberToDelete;
        default:
        break;
    }
}while(shouldContinue != 'Y');
return -1;
}

deleteARecord.h:

#ifndef DELETEARECORD_H_INCLUDED
#define DELETEARECORD_H_INCLUDED

void deleteARecord(void);

#endif // DELETEARECORD_H_INCLUDED

deleteARecord.c:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <ctype.h>
#include "deleteARecord.h"
#include "main.h"
void deleteARecord(){
    int numberToDelete;
    struct account *previousa;

    numberToDelete = promptForDelete();
    if(numberToDelete == -1)
        {
        printf("No items to delete - (-1 return)\n");
        return;
        }
    if (firsta == NULL)
    {
        printf("There are no items to delete!");
                return;
    }
    else
    {
        currenta = firsta;

        while(currenta != NULL)
        {
            if(currenta->number == numberToDelete)
            {
                if(currenta == firsta) /*Special case for first record because it has no previous record*/
                    firsta = currenta->next;
                else
                    previousa->next = currenta->next;
                free(currenta);
                printf("Account #%d deleted.\n", numberToDelete);
                listAll();
                return;
            }
            else
            {
                previousa = currenta;
                currenta = currenta->next;
            }
        }
        printf("Account #%d was not found.\n",numberToDelete);
        puts("Nothing deleted");

    }
}

modifyARecord.h:

#ifndef MODIFYARECORD_H_INCLUDED
#define MODIFYARECORD_H_INCLUDED

void modifyARecord(void);


#endif // MODIFYARECORD_H_INCLUDED

modifyARecord.c:

#include <stdio.h>
#include "modifyARecord.h"
#define TRUE 1
#define FALSE 0
#include "main.h"
/*This struct serves as an "object" to hold the numbers and a bool flag*/

struct accountNumbers{
    int numberToModify;
    int newNumber;
    unsigned int success;
};
struct accountNumbers *promptForModify(struct accountNumbers *accountNumbersPtr);

/**
 *
 *
 *  FUNCTION MODIFIES THE RECORD'S ID NUMBER
 *
 *
 */

void modifyARecord()
{

    struct account *previousa;
    struct accountNumbers *numbersPtr;
    struct accountNumbers theNumbers;
    theNumbers.newNumber = 0;
    theNumbers.numberToModify =0;
    theNumbers.success = FALSE;
    numbersPtr = &theNumbers;
    promptForModify(numbersPtr);
    if(numbersPtr->success == FALSE)
        {
            printf("CANCELLING...");
        }
    if (firsta == NULL)
    {
        printf("There are no items to modify!");
                return;
    }
    else
    {
        currenta = firsta;

        while(currenta != NULL)
        {
            if(currenta->number == numbersPtr->numberToModify)
            {
                currenta->number = numbersPtr->newNumber;
                printf("Account #%d updated.\n", numbersPtr->numberToModify);
                listAll();
                return;
            }
            else
            {
                previousa = currenta;
                currenta = currenta->next;
            }
        }
        printf("Account #%d was not found.\n",numbersPtr->numberToModify);
        puts("Nothing deleted");
    }
}

/**
 *
 *
 *  FUNCTION PROMPTS THE USER FOR THE NUMBER TO MODIFY AND THE NUMBER TO REPLACE WITH
 *
 *
 */
    struct accountNumbers* promptForModify(struct accountNumbers *accountNumbersPtr)
    {
        printf("Enter an ID number of the record you want to modify: ");
        scanf("%d",&accountNumbersPtr->numberToModify);
        printf("You entered %d, now enter the replacement number OR 0 to go back.",accountNumbersPtr->numberToModify);
        getchar();
        scanf("%i",&accountNumbersPtr->newNumber);
    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
        }
    }
\$\endgroup\$
  • 1
    \$\begingroup\$ You appear to have pasted the code for deleteARecord.h into your modifyARecord.h header. \$\endgroup\$ – forsvarir Dec 9 '16 at 10:35
  • 1
    \$\begingroup\$ Did you somehow destroy your indentation on posting, or is it really that interesting? \$\endgroup\$ – Deduplicator Dec 9 '16 at 10:51
  • \$\begingroup\$ whooops Ill fix that, accidentally pasted deleteARecord twice I guess. @Deduplicator Yeah I was having some issues when posting this for some reason, a lot of the code would not format correctly but which part specifically are you referring to? \$\endgroup\$ – the_endian Dec 9 '16 at 10:59
  • \$\begingroup\$ @TeeSee There's some pretty funky indentation in most of the files. If you copy/paste straight from your IDE, select the code and press 'ctrl + K', or click the code button '{}' it usually indents it the same as it is in your ide. \$\endgroup\$ – forsvarir Dec 9 '16 at 11:05
  • \$\begingroup\$ Your problem is that you are mixing tabs and spaces. Best onvert it all to spaces... \$\endgroup\$ – Deduplicator Dec 9 '16 at 15:27
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for this... This is exactly what I was looking for. I knew I was doing some bad stuff, but I didn't have the experience yet with C to pinpoint it with a clear head like you did. I'll try to think of way to do it with fewer globals and consolidate the functions to files in a more organized way. As well as heed your other advice, which all makes total sense! \$\endgroup\$ – the_endian Dec 9 '16 at 11:33
  • 1
    \$\begingroup\$ @TeeSee Glad it's useful. CR questions can take longer than questions on SO to attract answers, so I generally wait about a week before accepting an answer because having an accepted answer can discourage future reviews and with CRs different people will often pick up on different elements. Once you hit 15 rep you can of course upvote any and all useful answers. Alternately you might want to make changes and ask through an iterative review. General guidelines for followups are here: meta.codereview.stackexchange.com/a/1765/4203 \$\endgroup\$ – forsvarir Dec 9 '16 at 12:03
  • \$\begingroup\$ Thanks for that as well. I've been going down the answers here like a checklist and updating my application. Is it possible to do something like if(newa != NULL) {free(newa);} or is that not going to have the desired effect? \$\endgroup\$ – the_endian Dec 10 '16 at 4:01
  • \$\begingroup\$ @TeeSee you could do that, however it would be better to only allocate the memory when required. You would typically assign the variable to null after freeing it as well, in order to prevent accidentally referencing the memory afterwards. \$\endgroup\$ – forsvarir Dec 10 '16 at 8:50
  • \$\begingroup\$ oh yeah plus I forgot that a memory leak really has nothing to do with the pointer itself... Is the fact that I am exiting the program without walking the linked list and free()ing every single bit of memory a bad thing on modern OS's? I forgot that technically I have not done that and so every time the program exits, technically all of the linked list is a memory leak. \$\endgroup\$ – the_endian Dec 10 '16 at 23:30
2
\$\begingroup\$

I just go from top to bottom:

1. Includes

  • The _INCLUDED is usually not written. Of course, you can name it whatever you like but in production code you would normally not see _INCLUDED

2. Public/Private

  • Think about which functions are public and private. Put public functions declarations in the header file and private functions as static functions in the c file. (information hiding, as you already pointed out)

3. struct account

  • I'd create a typedef struct for that. It's easier to use.
  • In C the size of int is not constant over different platforms. Therefor you want to use a typedef, e.g. typedef int int32_t to indicate that its a 32 bit value. In the particular case of an account number it is probably even a unsigned int
  • float has only 6 defined digits, it can quickly run out of precision and in particular if you are dealing with money that's a highly unwanted effect. ;-)

4. Linked list

  • I wouldnt link the accounts in the account itself. The account should only deal with things that are related to the account. E.g. a customer can have several accounts but an account shouldn't have any knowledge about other accounts.
  • I think a linked list is not an ideal data structure to store a large number of accounts, since access times are bad O(n) you could use a hash table.

Style

  • In C you generally start a new line after { (but that's debatable), you should try to not mix styles.
  • Check your indentation, e.g. loadRecorsFromFile() isn't indented
  • Use opening end closing brackets { } for if statements with only one instruction. It may lead to bugs if you don't and its just easier to read.

Switch statement

  • you could use a table to handle the commands, of course if you only have a couple of commands it wouldn't be worth it but if you want to keep your design extendable you probably want to use a table or even tables if you have sub commands

Variables

  • Dont keep globals which are used in the functions. You should work with function arguments. E.g. err_t saveRecordsToFile(records_t *myRecords)

Architecture

  • You could improve your architecture by using following class diagram. Each class would be represented by a .c and .h file. A bank can have several customers and a customer can have accounts.

class diagram

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot, wish my upvote would show up already so I can upvote yours! Re typedef int int32_t point - So basically if I don't do this and just use a plain old int, and the system the program is on thinks that an int is like default 8 or 16 instead of 32, it can cause a buffer overflow or just basically max it out/truncate it unintentionally? And re: architecture. So when you say that a bank "would have," in C that translates to has pointers to structs of customers? I'm familiar with this in OOP but new to C still. Thanks again! \$\endgroup\$ – the_endian Dec 10 '16 at 3:29
  • \$\begingroup\$ sizeof(int): yes you got it, see the answer for more details stackoverflow.com/questions/11438794/… Yes the bank would create a customer and keep a pointer to it and keep track of them. \$\endgroup\$ – Frode Akselsen Dec 10 '16 at 4:28
  • \$\begingroup\$ btw leanpub.com/patternsinc has a nice example of "object oriented" programming in c. Its not free though. It makes your C code much better readable, reusable and if you use automated testing (e.g. cppunit) you definitely want to use it. \$\endgroup\$ – Frode Akselsen Dec 10 '16 at 4:58
  • \$\begingroup\$ thanks yeah I wanted to start to incorporate unit tests and possibly TDD in my C code since I did that in C#. I had found CUnit or something like that but I honestly was not sure if unit testing was a widespread thing or desired in the C World although I don't see how it could hurt that's for sure. I would like to do some TDD in C. I'm not trying to make C into OOP (in fact I try to keep it totally separate from C++)but in a way, we could say that C still uses objects as structs so that seems pretty natural. Thanks again! \$\endgroup\$ – the_endian Dec 10 '16 at 6:03

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.