3
\$\begingroup\$

This is a school assignment, it's the longest program I've wrote this far, and I feel it's a bit messy and all over the place. I will try to translate the assignment to the best I can:

We have a binary file for cars, each car struct contains (Car ID, manufacturing year, colour, an array with the IDs of this car's owners). A car can have a maximum of 5 owners.

We have another binary file with owner information, each struct contains (Owner ID, and name).

  1. Write a function to add a new owner (making sure they don't exist already).
  2. Write a function to add a new car to the car file
  3. Delete a car by ID (and delete each owner of this car that doesn't have another car)
  4. Create a linked list for white cars with ascending order.
  5. Delete all cars from the linked list above that have a manufacturing year before 1980

The first 2 functions are either called from inside main, or from one another. If they're called from main, their argument is 0.

The paths they can take:

main() > add_owner(0) > add a new car for this owner add_car(owner_ID), or link an already existing car

And

main() > add_car(0) > add a new owner for this car add_owner(car_ID), or link this car to an already existing owner.

The third function takes an argument of the ID of the car you want to delete, it will search the car file, it it does find it, then it stores it in struct1, then:

  1. Create a secondary struct, and rewind the car file.
  2. Test the i element inside the owners array of the car I want to delete (struct1) against all other owners inside the car file (struct2) (making sure to skip the desired car)
  3. If the element is found (i.e the owner has more than one car), then delete it from the owners array inside struct1, rewind the file and start with the next element.
  4. If the element is not found, keep it in the array and rewind the file, move on to the next element.

This leaves me with the owners who have a single car, then I delete them from the owners file, and in the end, I delete the desired car from the car file.

The 4th and 5th questions don't need much explanation, I convert all of the cars' colour names to lower character, in order to make comparison with white easier. When creating a linked list, I check if a node already exists before inserting.

While all input is appreciated, I'm mainly looking for possible bugs, something that can make the program crash, or a minor thing I forgot.

The program compiles fine with a single warning about control reaching nonvoid function or something.

#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>
#include <string.h>
#define MAX_ARRAY_SIZE 50
#define CURRENT_YEAR 2017

const char owner_filename[] = "owners";
const char car_filename[] = "car";

struct owner{
    long owner_ID;
    char name[MAX_ARRAY_SIZE];
};

struct car{
    long car_ID;
    int year; /*Manufacturing year*/
    int n_owners; /*Current number of owners*/
    char colour[MAX_ARRAY_SIZE];
    int owners_ID[5]; /*A car can only have 5 max owners*/
};

struct node{
    struct car car_node;
    struct node *next_ptr;
};

struct node *head = NULL;

long add_owner(long car_ID);
void add_car(long owner_ID);
void link_car(long owner_ID); /*Link car to owner*/
long link_owner(void); /*Link owner to a car*/
void create_linked_list(void); /*Create a linked list of white cars*/
void insert(struct car white_car);
int node_already_exists(struct car white_car); /*Check if a car already exists in the linked list*/
void delete_car(void); /*Delete a car based on its ID from the file*/
void delete_cars_from_list(void); /*Delete cars based on their date from the linked list*/
void clear(void);  /*Clear standard input*/

int main(void){
    int c;

    printf("Press 'c' to add a new car\nPress 'a' to add a new owner\nPress 'd' to delete a car\nPress 'l' to create a linked list of white cars\nPress 'r' to remove cars made before 1980 from the linked list\n");
    while((c = getchar()) != EOF){
        switch(c){
        case 'a':
            clear();
            add_owner(0);
            break;
        case 'c':
            clear();
            add_car(0);
            break;
        case 'd':
            clear();
            delete_car();
            break;
        case 'l':
            clear();
            create_linked_list();
            break;
        case 'r':
            clear();
            delete_cars_from_list();
            break;
        default:
            clear();
            printf("Invalid command: %c\n", c);
            break;
        }
        puts("Press 'c' to add a new car, 'a' to add a new owner, 'd' to delete a car, 'l' to create a linked list, 'r' to remove cars from the linked list");
    }
    return 0;
}

long add_owner(long car_ID){
    FILE *binary_file;

    if((binary_file = fopen(owner_filename, "a+b")) == NULL){
        perror(owner_filename);
        exit(EXIT_FAILURE);
    }

    struct owner holder;

    puts("Enter the owner's ID");

    if(1 != scanf("%li", &holder.owner_ID)){
        fprintf(stderr, "Unable to read number");
        exit(EXIT_FAILURE);
    }
    clear();

    while(holder.owner_ID <= 0){
        puts("The ID cannot be 0 or less. Please enter a new ID");
        if(1 != scanf("%li", &holder.owner_ID)){
            fprintf(stderr, "Unable to read number");
            exit(EXIT_FAILURE);
        }
        clear();
    }

    puts("Enter the owner's name");
    fgets(holder.name, sizeof(holder.name), stdin);

    struct owner temp;

    while((fread(&temp, sizeof(temp), 1, binary_file)) != 0){
        if(holder.owner_ID == temp.owner_ID){
            puts("Owner already exists");
            return -1;
        }
    }

    fwrite(&holder, sizeof(holder), 1, binary_file);
    fclose(binary_file);
    puts("Added new owner successfully");

    if(car_ID > 0){
        return holder.owner_ID;
    }
    else{
        puts("Press 'n' to add a new car for this owner, or 'e' to link him to an already existing car");
        int c;
        while((c = getchar()) != EOF){
            switch(c){
            case 'n':
                clear();
                add_car(holder.owner_ID);
                break;
            case 'e':
                clear();
                link_car(holder.owner_ID);
                break;
            default:
                clear();
                printf("Invalid command: %c\n", c);
                break;
            }
            puts("Press 'n' to add a new car for this owner, or 'e' to link him to an already existing car");
        }
    }
}


void add_car(long owner_ID){
    FILE *binary_file;

    if((binary_file = fopen(car_filename, "a+b")) == NULL){
        perror(car_filename);
        exit(EXIT_FAILURE);
    }

    struct car new_car;
    new_car.n_owners = 0;

    puts("Enter the car's ID");

    if(1 != scanf("%li", &new_car.car_ID)){
        fprintf(stderr, "Unable to read number");
        exit(EXIT_FAILURE);
    }
    clear();

    while(new_car.car_ID <= 0){
        puts("The ID cannot be 0 or less. Please enter a new ID");
        if(1 != scanf("%li", &new_car.car_ID)){
            fprintf(stderr, "Unable to read number");
            exit(EXIT_FAILURE);
        }
        clear();
    }

    puts("Enter the year of the making");

    if(1 != scanf("%i", &new_car.year)){
        fprintf(stderr, "Unable to read number");
        exit(EXIT_FAILURE);
    }
    clear();

    while((new_car.year < 1885) || (new_car.year > CURRENT_YEAR)){
        puts("The year of the making can only be between 1885 and the current year");
        if(1 != scanf("%i", &new_car.year)){
            fprintf(stderr, "Unable to read number");
            exit(EXIT_FAILURE);
        }
        clear();
    }

    puts("Enter the colour of the car");
    fgets(new_car.colour, sizeof(new_car.colour), stdin);

    for(int i = 0; new_car.colour[i] != '\n'; ++i){
        if(!(isalpha(new_car.colour[i]))){
            fprintf(stderr, "Invalid character: %c\n", new_car.colour[i]);
            exit(EXIT_FAILURE);
        }
        new_car.colour[i] = tolower(new_car.colour[i]);
    }

    struct car temp;

    while((fread(&temp, sizeof(temp), 1, binary_file)) != 0){
        if(new_car.car_ID == temp.car_ID){
            puts("Car already exists");
            return;
        }
    }

    if(owner_ID > 0){
        new_car.owners_ID[0] = owner_ID;
        new_car.n_owners++;
        fwrite(&new_car, sizeof(new_car), 1, binary_file);
        puts("Added new car successfully to this owner");
        fclose(binary_file);
        return;
    }

    puts("Press 'n' to add a new owner for this car, or 'e' to link it to an already existing owner");
    int c;
    while((c = getchar()) != EOF){
        switch(c){
        case 'n':
            clear();
            if(new_car.n_owners < 5){
                long owner_ID = add_owner(new_car.car_ID);
                if(owner_ID > 0){
                    new_car.owners_ID[new_car.n_owners] = owner_ID;
                    new_car.n_owners++;
                }
            }
            else{
                puts("This car already has 5 owners");
            }
            break;
        case 'e':
            clear();
            if(new_car.n_owners < 5){
                long link_owner_ID = link_owner();
                if(link_owner_ID > 0){
                    new_car.owners_ID[new_car.n_owners] = link_owner_ID;
                    new_car.n_owners++;
                    puts("Owner linked successfully");
                }
            }
            else{
                puts("This car already has 5 owners");
            }
            break;
        default:
            clear();
            printf("Invalid command: %c\n", c);
            break;
        }
        puts("Press 'n' to add a new owner for this car, or 'e' to link it to an already existing owner");
    }

    fwrite(&new_car, sizeof(new_car), 1, binary_file);
    fclose(binary_file);
    puts("Added new car successfully");
}


void link_car(long owner_ID){
    FILE *binary_file;

    if((binary_file = fopen(car_filename, "rb")) == NULL){
        perror(car_filename);
        exit(EXIT_FAILURE);
    }

    FILE *copy;

    if((copy = fopen("copy", "wb")) == NULL){
        perror("copy");
        exit(EXIT_FAILURE);
    }

    struct car linked_car;

    puts("Enter the ID of the car you want to link this owner to");
    long link_ID;
    if(1 != scanf("%li", &link_ID)){
        fprintf(stderr, "Unable to read number");
        exit(EXIT_FAILURE);
    }
    clear();

    while(link_ID <= 0){
        puts("The ID cannot be 0 or less. Please enter a new ID");
        if(1 != scanf("%li", &link_ID)){
            fprintf(stderr, "Unable to read number");
            exit(EXIT_FAILURE);
        }
        clear();
    }

    while((fread(&linked_car, sizeof(linked_car), 1, binary_file)) != 0){
        if(linked_car.car_ID == link_ID){
            if(linked_car.n_owners >= 5){
                puts("This car already has maximum number of owners");
                return;
            }
            else{
                rewind(binary_file);
                while((fread(&linked_car, sizeof(linked_car), 1, binary_file)) != 0){
                    if(linked_car.car_ID == link_ID){
                        linked_car.owners_ID[linked_car.n_owners] = owner_ID;
                        linked_car.n_owners++;
                        fwrite(&linked_car, sizeof(linked_car), 1, copy);
                        continue;
                    }
                    fwrite(&linked_car, sizeof(linked_car), 1, copy);
                }

                fclose(binary_file);
                fclose(copy);

                if((binary_file = fopen(car_filename, "wb")) == NULL){
                    perror(car_filename);
                    exit(EXIT_FAILURE);
                }

                if((copy = fopen("copy", "rb")) == NULL){
                    perror("copy");
                    exit(EXIT_FAILURE);
                }

                while((fread(&linked_car, sizeof(linked_car), 1, copy)) != 0){
                    fwrite(&linked_car, sizeof(linked_car), 1, binary_file);
                }
                fclose(binary_file);
                fclose(copy);
                remove("copy");
                puts("Linked car to owner successfully");
                return;
            }
        }
    }
    fclose(binary_file);
    fclose(copy);
    remove("copy");
    puts("Car not found");
}


long link_owner(void){
    FILE *binary_file;

    puts("Enter the ID of the owner you want to link this car to");
    long owner_ID;

    if(1 != scanf("%li", &owner_ID)){
        fprintf(stderr, "Unable to read number");
        exit(EXIT_FAILURE);
    }
    clear();

    while(owner_ID <= 0){
        puts("The ID cannot be 0 or less. Please enter a new ID");
        if(1 != scanf("%li", &owner_ID)){
            fprintf(stderr, "Unable to read number");
            exit(EXIT_FAILURE);
        }
        clear();
    }

    if((binary_file = fopen(owner_filename, "rb")) == NULL){
        perror(owner_filename);
        exit(EXIT_FAILURE);
    }

    struct owner temp;

    while((fread(&temp, sizeof(temp), 1, binary_file)) != 0){
        if(owner_ID == temp.owner_ID){
            return owner_ID;
        }
    }
    fclose(binary_file);
    puts("Owner not found");
    return -1;
}

void delete_car(void){
    puts("Enter the ID of the car you want to delete");
    long delete_cID;

    if(1 != scanf("%li", &delete_cID)){
        fprintf(stderr, "Unable to read number");
        exit(EXIT_FAILURE);
    }

    while(delete_cID <= 0){
        puts("The ID cannot be 0 or less. Please enter a new ID");
        if(1 != scanf("%li", &delete_cID)){
            fprintf(stderr, "Unable to read number");
            exit(EXIT_FAILURE);
        }
        clear();
    }
    FILE *binary_file;

    if((binary_file = fopen(car_filename, "rb")) == NULL){
        perror(car_filename);
        exit(EXIT_FAILURE);
    }

    struct car temp;

    int found_car = 0;
    while((fread(&temp, sizeof(temp), 1, binary_file)) != 0){
        if(temp.car_ID == delete_cID){
            found_car = 1;
            break;
        }
    }

    if(found_car == 0){
        puts("Car not found");
        return;
    }

    struct car other_cars;
    rewind(binary_file);

    for(int i = 0; i < temp.n_owners; ++i){
        while((fread(&other_cars, sizeof(other_cars), 1, binary_file)) != 0){
            if(other_cars.car_ID == delete_cID){
                continue;
            }
            for(int j = 0; j < other_cars.n_owners; ++j){
                if(temp.owners_ID[i] == other_cars.owners_ID[j]){
                    for(int k = i; k < temp.n_owners; ++k){
                        temp.owners_ID[k] = temp.owners_ID[k+1];
                    }
                    temp.n_owners--;
                    --i;
                    break;
                }
            }
        }
        rewind(binary_file);
    }

    for(int i = 0; i < temp.n_owners; ++i){
        printf("the IDs left are%i\n", temp.owners_ID[i]);
    }
    fclose(binary_file);

    FILE *owner_file;

    struct owner owner_temp;

    if((owner_file = fopen(owner_filename, "rb")) == NULL){
        perror(owner_filename);
        exit(EXIT_FAILURE);
    }

    FILE *owner_copy_ptr;

    if((owner_copy_ptr = fopen("owner_copy", "wb")) == NULL){
        perror("owner_copy");
        exit(EXIT_FAILURE);
    }

    int flag = 1;
    while(fread(&owner_temp, sizeof(owner_temp), 1, owner_file) != 0){
        flag = 1;
        for(int i = 0; i < temp.n_owners; ++i){
            if(owner_temp.owner_ID == temp.owners_ID[i]){
                flag = 0;
                break;
            }
        }
        if(flag == 1){
            fwrite(&owner_temp, sizeof(owner_temp), 1, owner_copy_ptr);
        }
    }


    fclose(owner_file);
    fclose(owner_copy_ptr);

    if((owner_file = fopen(owner_filename, "wb")) == NULL){
        perror(owner_filename);
        exit(EXIT_FAILURE);
    }

    if((owner_copy_ptr = fopen("owner_copy", "rb")) == NULL){
        perror("owner_copy");
        exit(EXIT_FAILURE);
    }

    while(fread(&owner_temp, sizeof(owner_temp), 1, owner_copy_ptr) != 0){
        fwrite(&owner_temp, sizeof(owner_temp), 1, owner_file);
    }

    fclose(owner_file);
    fclose(owner_copy_ptr);
    remove("owner_copy");
    /*Edited owner file*/

    if((binary_file = fopen(car_filename, "rb")) == NULL){
        perror(car_filename);
        exit(EXIT_FAILURE);
    }

    FILE *car_copy_ptr;

    if((car_copy_ptr = fopen("car_copy", "wb")) == NULL){
        perror("car_copy");
        exit(EXIT_FAILURE);
    }

    while((fread(&other_cars, sizeof(other_cars), 1, binary_file)) != 0){
        if(other_cars.car_ID == delete_cID){
            puts("found car");
            continue;
        }
        fwrite(&other_cars, sizeof(other_cars), 1, car_copy_ptr);
    }

    fclose(binary_file);
    fclose(car_copy_ptr);

    if((binary_file = fopen(car_filename, "wb")) == NULL){
        perror(car_filename);
        exit(EXIT_FAILURE);
    }

    if((car_copy_ptr = fopen("car_copy", "rb")) == NULL){
        perror("car_copy");
        exit(EXIT_FAILURE);
    }

    while((fread(&other_cars, sizeof(other_cars), 1, car_copy_ptr)) != 0){
        fwrite(&other_cars, sizeof(other_cars), 1, binary_file);
    }
    fclose(binary_file);
    fclose(car_copy_ptr);
    remove("car_copy");
}


void create_linked_list(){
    FILE *binary_file;

    if((binary_file = fopen(car_filename, "rb")) == NULL){
        perror(car_filename);
        exit(EXIT_FAILURE);
    }

    struct car temp;
    const char insert_colour[] = "white";

    int flag = 0;
    while((fread(&temp, sizeof(temp), 1, binary_file)) != 0){
        if(strstr(temp.colour, insert_colour)){
            if(!node_already_exists(temp)){
                insert(temp);
            }
            flag = 1;
        }
    }
    if(flag){
        puts("Created linked list successfully");
    }
    else{
        puts("No white cars were found");
    }
    fclose(binary_file);
}

void insert(struct car white_car){
    struct node *temp = head;
    struct node *prev;
    struct node *new_node;

    if(head == NULL){
        head = malloc(sizeof(struct node));
        head->car_node = white_car;
        head->next_ptr = NULL;
        return;
    }

    if(head->car_node.year > white_car.year){
        new_node = malloc(sizeof(struct node));
        new_node->car_node = white_car;
        new_node->next_ptr = head;
        head = new_node;
        return;
    }

    while((temp != NULL) && (temp->car_node.year < white_car.year)){
        prev = temp;
        temp = temp->next_ptr;
    }

    new_node = malloc(sizeof(struct node));
    new_node->car_node = white_car;
    new_node->next_ptr = temp;
    prev->next_ptr = new_node;
}

int node_already_exists(struct car white_car){
    struct node *temp = head;

    while(temp != NULL){
        if(temp->car_node.car_ID == white_car.car_ID){
            return 1;
        }
        temp = temp->next_ptr;
    }
    return 0;
}

void delete_cars_from_list(void){
    struct node *temp;

    if(head == NULL){
        puts("Linked list is empty");
        return;
    }

    while(head->car_node.year < 1980){
        temp = head;
        if(head->next_ptr == NULL){
            head = NULL;
            free(temp);
            return;
        }
        else{
            head = head->next_ptr;
            free(temp);
        }
    }

    struct node *cur = head;
    struct node *prev = NULL;

    while(cur != NULL){
        if(cur->car_node.year < 1980){
            temp = cur;
            prev->next_ptr = cur->next_ptr;
            cur = cur->next_ptr;
            free(temp);
            puts("Node Deleted...");
            continue;
        }
        prev = cur;
        cur = cur->next_ptr;
    }
}


void clear(void){
    int c;
    while((c = getchar()) != '\n' && (c != EOF));
}
\$\endgroup\$
6
  • \$\begingroup\$ with a single warning about control reaching nonvoid function It warns about reaching the end of a non-void function. In this case the return value is probably undefined which is not a good idea. \$\endgroup\$
    – yuri
    Commented Jun 2, 2017 at 16:39
  • \$\begingroup\$ @yuri The return value in that case is not used, so I didn't bother with the warning. \$\endgroup\$
    – user127566
    Commented Jun 2, 2017 at 17:35
  • \$\begingroup\$ I see 2 problems with your answer to @yuri, one you sometimes but not always check the return value of a particular function, two it is generally not a good idea to ignore warning messages. Some people that want to write better code turn the warning messages into hard errors with a compiler switch. The fact that the function may return values that are ignored indicates there should be two functions, one that returns values and one that does not return values. You may be trying to do too much with one function. \$\endgroup\$
    – pacmaninbw
    Commented Jun 2, 2017 at 23:22
  • 1
    \$\begingroup\$ should not call clear() when c == '\n' \$\endgroup\$
    – chux
    Commented Jun 3, 2017 at 2:42
  • 1
    \$\begingroup\$ @MichaelX Yes there will be something after '\n', the next line of input which clear() will consume - even if valid. Try entering a valid "r\n"after a blank line with your code. \$\endgroup\$
    – chux
    Commented Jun 3, 2017 at 12:29

2 Answers 2

1
\$\begingroup\$

Bugs in the Code

  • Bug in link_owner() when the owner is found the binary file is never closed.
  • Bug in add_car() there is no code to handle what to do if link_owner() returns -1. (missing possible error message).
  • Bug in delete_cars_from_list(), the first time through the second while loop prev will be NULL when prev->next_ptr is referenced, this will result in unknown behavior. The second while loop should not be needed since the first loop should remove all the necessary cares. Based on the possible bug it is clear this code is never being executed.

Possible Bugs
The calls to malloc() are not tested. The functions malloc() and calloc can fail when too much memory is allocated. When they fail the value returned is NULL, access to fields from a NULL pointer result in unknown errors, but it generally terminates the program, it is a good idea to always test the return value from malloc() or calloc() prior to trying to use the pointer. It is possible that the code in the insert() function should changed in the following manner:

void insert(struct car white_car)
{
    struct node *temp = head;
    struct node *prev;
    struct node *new_node = malloc(sizeof(*new_node)

    if (new_node == NULL)
    {
        fprintf(stderr, "ERROR: malloc failed in insert\n");
        return;
    }
    new_node->car_node = white_car;
    new_node->next_ptr = NULL;

    if(head == NULL){
        head = new_node;
        return;
    }

    if(head->car_node.year > white_car.year){
        new_node->next_ptr = head;
        head = new_node;
        return;
    }

    while((temp != NULL) && (temp->car_node.year < white_car.year)){
        prev = temp;
        temp = temp->next_ptr;
    }

    new_node->next_ptr = temp;
    prev->next_ptr = new_node;
}

Programs should always clean up after themselves, there is no code to close all files at the end of the program.

In the struct car, the owners_ID array should be long rather than int. These lines give warnings when I compile the program, there is no guarantee that long and int will be the same size on all platforms. The type int can be as few as 16 bits and as large as the word size gets. If the mismatch continues use an explicit cast from long to int

linked_car.owners_ID[linked_car.n_owners] = (int) owner_ID;

As noted by @yuri above, the warning message indicates that there are paths through add_car() that don't return values, this actually indicates there can be bugs in the function.

Magic Numbers
The code already has a few symbolic constants, but closely examining the code shows that there are still some numeric constants that should be converted to symbolic constants:

  • There should be a symbolic name for 1980
  • There should be a symbolic name for 5 (MAX_OWNERS).
  • There should be a symbolic name for 1985. Should 1885 be 1980 or 1985?

The other answer by @chux provides some good input on this as well.

Global Variables
It's best to avoid global variables whenever possible for a number of reasons, and there is a good discussion of this on stack overflow. Some of the problems associated with global variables are that they are hard to debug, they can be changed anywhere in the code, and it is hard to track down where they are changed. They make writing code more dificult and reading code much more difficult (in the case of this program, you need to look through 661 lines of code to find where the values of the global variables change). If the program is larger that this one, and in multiple 1000 line files, it can be really difficult to find where a variable is being changed. They may be redeclared in other files and the problem will show up at link time.

It's better to have as few global variables as possible and pass by reference or value whenever a variable needs to be used by a function. Pass by reference will prevent certain kinds of errors such as changing a global value when the change should only be local.

Reduce Complexity, Follow SRP, and KIS(S)
The Single Responsibility Principle states that every module or class should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by the class. All its services should be narrowly aligned with that responsibility.

Robert C. Martin expresses the principle as follows:
    `A class should have only one reason to change.`

While this is primarily targeted at classes in object oriented languages it applies to functions and subroutines in procedural languages like C as well.

The KIS(S) principle is simply stated as Keep It Simple and is used in all forms of engineering.

  • The function delete_car() is way too long and overly complex, it needs to be broken into many sub functions.
  • The function add_car() is way too long and overly complex, it needs to be broken into many sub functions.

The sub functions of both of these functions may be usable and sharable by each of the high level functions and that may reduce the total lines of code in this program.

Error Handling in C
There are times when it is inadvisable to call exit(EXIT_FAILURE) either because the code has necessary clean up that it must do, or the code is part of a larger system that should not quit such as an operating system. In such cases the error should be reported and then either the function should return an error status or some form of exception handling should be used. In the C Program Language the only exception handling is the possibility of returning to a known good state using setjmp() and longjmp(). An example use of setjmp() and longjmp() can be found here.

The function setjmp() saves the state of the program at the location where it is called and provides a location to jump back to in case of errors. The function longjmp() jumps to the location indicated by setjmp().

In best place to put a call to setjmp() is in a high level function such as main() or one of the functions called directly by main()

Alternate Form of Main
In more complex C programs such as this one it is better to limit the code in the main() function to setting up the program to run such as parsing the command line for switches and opening any input files, calling a function that executes the major portion of the program and then calling functions that clean up after the program has run, such as freeing any memory that has been allocated or writing to and closing any files that have been open.

An example:

int menu_loop(struct car_node* cars_list, struct owner_node* owners_list)
{
    int status = EXIT_SUCCESS;
    jmp_buf env;

    if (setjmp(env) != 0)
    {
        return EXIT_FAILURE;
    }

    printf("Press 'c' to add a new car\nPress 'a' to add a new owner\nPress 'd' to delete a car\nPress 'l' to create a linked list of white cars\nPress 'r' to remove cars made before 1980 from the linked list\n");
    int c;
    while(((c = getchar()) != EOF) && (status == EXIT_SUCCESS))
    {
        switch(c){
            case 'a':
                if (add_owner(0, owners_list) < 0)
                {
                    status = EXIT_FAILURE;
                }
                break;
            case 'c':
                if (add_car(0, car_list) < 0)
                {
                    status = EXIT_FAILURE;
                }
                break;
            case 'd':
                if (delete_car(car_list) < 0)
                {
                    status = EXIT_FAILURE;
                }
                break;
            case 'r':
                if (delete_cars_from_list(car_list) < 0)
                {
                    status = EXIT_FAILURE;
                }
                break;
            default:
                printf("Invalid command: %c\n", c);
                break;
        }
        puts("Press 'c' to add a new car, 'a' to add a new owner, 'd' to delete a car, 'l' to create a linked list, 'r' to remove cars from the linked list");
    }
    return status;
}

int main(int argc, char* argv[])
{
    int status = EXIT_SUCCESS;
    const char owner_filename[] = "owners";
    const char car_filename[] = "cars";
    struct owner_node* owners_list = NULL;
    struct car_node* car_list = NULL;

    owners_list = load_owner_list_from_file(owner_filename);
    if (!owners_list)
    {
        status = EXIT_FAILURE;
    }

    if (status == EXIT_SUCCESS)
    {
        car_list = load_car_list_from_file(car_filename);
        if (!car_list)
        {
            status = EXIT_FAILURE;
        }
    }

    if (status == EXIT_SUCCESS)
    {
        status = execute_main_loop(car_list, owners_list);
    }

    write_lists_to_files_delete_lists(car_list, owners_list, car_filename, owner_filename);

    return status;
}
\$\endgroup\$
2
  • \$\begingroup\$ Thanks, I didn't expect anyone to read the entire thing. I've already presented the project and had fixed the bugs you mentioned already, the other points you mention are fair. Except the malloc when, I've been told it's not worth it to check the return value if you're only allocating a few bytes. \$\endgroup\$
    – user127566
    Commented Jun 4, 2017 at 19:45
  • \$\begingroup\$ @MichaelX One thing to consider, the code would be better if more of the data was kept in memory, each call to fopen(), fclose(), fread(), fwrite() and readwind() are system calls which means the program is swapped out while they are executing. One of the ways to improve performance would be to read all the data into memory from the files, manipulate it in memory and then write all the changes back out to the files. An alternate would be to build a table of indexes into the file and use fseek to find all the records. Both methods consume more memory than you might assume. \$\endgroup\$
    – pacmaninbw
    Commented Jun 5, 2017 at 11:47
1
\$\begingroup\$

Mostly small stuff

  1. To insure output is seen before read, flush the output buffer after writing to stdout, especially if the last character is not a '\n'.

    puts("Enter the owner's ID");
    fflush(stdout); // add
    if(1 != scanf("%li", &holder.owner_ID)){
    
  2. Rather than #define CURRENT_YEAR 2017, extract current year from __DATE__ or time().

Minor

Was the right type used? Why bother with making sure? Use the size of the referenced object, easier to code maintain and review.

// new_node = malloc(sizeof(struct node)); // maybe wrong size
new_node = malloc(sizeof *new_mode);       // certainly correct.

() not needed in various places

// while((fread(&temp, sizeof(temp), 1, binary_file)) != 0){
while((fread(&temp, sizeof temp, 1, binary_file)) != 0){

is...() not defined for negative values aside from EOF

// ... isalpha(new_car.colour[i])
... isalpha((unsigned char) new_car.colour[i])

Given new car intros are often listed as next year:

// new_car.year > CURRENT_YEAR
new_car.year > (CURRENT_YEAR + 1)

MAX_ARRAY_SIZE is too generic. Consider #define OWNER_NAME_SIZE and #define COLOUR_SIZE

More C idiomatic to pass by address than value and looks like a good place for bool.

// int node_already_exists(struct car white_car); 
bool node_already_exists(const struct car *white_car); 
\$\endgroup\$
2
  • \$\begingroup\$ Don't you think the return result of malloc() should be tested before use? \$\endgroup\$
    – pacmaninbw
    Commented Jun 3, 2017 at 15:09
  • \$\begingroup\$ @pacmaninbw My review did not address that issue either way. Suggest adding your review of that as part of an answer. \$\endgroup\$
    – chux
    Commented Jun 3, 2017 at 17:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.