6
\$\begingroup\$

I'm relatively new to C programming and currently tackling exercises on Advent of Code. The challenge I'm working on involves calculating the sum of integers within each group from a file and identifying the top 3 groups based on their sums.

Here's an example of the input format:

group1 value 1
group1 value 2

group2 value 1

group3 value 1
group3 value 2

I've implemented the following code, where I iterate through the input character by character, calculating the sum for each row, and then tallying up the sums for each group. Additionally, I'm keeping track of whether a group is in the top 3.

#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>

void closeFile(FILE *file) {
    if (file != NULL) {
        fclose(file);
    }
}

void handleFileError(FILE *file, const char *errorMessage) {
    perror(errorMessage);
    closeFile(file);
    exit(EXIT_FAILURE);
}

void handleMemoryError(void *memory) {
    perror("Memory allocation error");
    free(memory);
    exit(EXIT_FAILURE);
}

size_t readFile(const char *filename, char **buffer) {
    FILE *file = fopen(filename, "rb");

    if (file == NULL) {
        handleFileError(file, "Error opening file");
    }

    fseek(file, 0, SEEK_END);
    long fileSize = ftell(file);
    fseek(file, 0, SEEK_SET);

    *buffer = (char *)malloc(fileSize + 1);
    if (*buffer == NULL) {
        handleMemoryError(*buffer);
    }

    size_t bytesRead = fread(*buffer, 1, fileSize, file);
    if (bytesRead != fileSize) {
        handleFileError(file, "Error reading file");
    }

    closeFile(file);

    (*buffer)[fileSize] = '\0';

    return bytesRead;
}

int main(int argc, char* argv[]) {    
    char *buffer = NULL;
    size_t fsize = readFile(argv[1], &buffer);
    
    bool line_changed = false; 
    int max1 = 0;
    int max2 = 0;
    int max3 = 0;
    int cum = 0;
    int line = 0;
    for (int i = 0; i < fsize; i++) {
        int j = buffer[i] - '0';
        if(j>=0){
            line_changed = false;
            line*=10;
            line+=j;
        }else if(!line_changed){
            line_changed = true;
            cum += line;
            line = 0;
        }else{
            line_changed = false;
            if(cum >= max1){
                max3 = max2;
                max2 = max1;
                max1 = cum;
            }else if(cum>=max2){
                max3 = max2;
                max2 = cum;
            }else if(cum >= max3){
                max3 = cum;
            }
            cum=0;
        }
    }
    if(cum>=max1){
        max2 = max1;
        max3 = max2;
        max1 = cum;
    }else if(cum>=max2){
        max3 = max2;
        max2 = cum;
    }else if(cum >= max3){
        max3 = cum;
    }
    printf("%i\n",max1);
    printf("%i\n",max1+max2+max3);
    free(buffer);

    return 0;
}

I would appreciate any suggestions for optimizing the code for readability and adhering to best practices.

\$\endgroup\$
8
  • 2
    \$\begingroup\$ Should we consider file handling in the answers? Because every time I see a complete file being copied to memory something in me dies. And since I'm expert on applied cryptography the whole thing is at least copied once more to create the ciphertext, so I die twice every post. \$\endgroup\$ Jan 24 at 23:42
  • \$\begingroup\$ I'm not sure if the code works actually. You don't seem to add the given values, you add together character values of each line (supposedly). If this does work I would expect comments in the code why this works, because without that the code is next to impossible to comprehend let alone validate. \$\endgroup\$ Jan 24 at 23:51
  • \$\begingroup\$ @MaartenBodewes The code computes correctly. In a very obfuscated and confusing way though. I don't think any comments amy salvage it. The complete redesign is in order of the day. \$\endgroup\$
    – vnp
    Jan 25 at 1:02
  • \$\begingroup\$ @MaartenBodewes Should we consider file handling in the answers? Of course yes. In this exchange every aspect of the code is a fair game. Just like in a professional code review. \$\endgroup\$
    – vnp
    Jan 25 at 1:10
  • \$\begingroup\$ @vnp I meant more in the sense that I was wondering if loading the file into memory was part of the assignment or that it was just test data considered to be in memory in the first place. \$\endgroup\$ Jan 25 at 1:32

2 Answers 2

4
\$\begingroup\$
  • More functions please....

    main does too much. Working over each individual elf shall be a function (returning the total calories it carries). Converting a line into a value shall also be a function (which BTW already exist in stdlib).

  • ... but not too much.

    I don't see a value in closeFile. If fopen failed, you return immediately, and in the rest of the program you are guaranteed that file != NULL. Which means that closeFile is just an obscure call to fclose.

  • There is no reason to read the entire file at once. Even if you insist on handling one character at a time, buffer[i] is no better than fgetc. The performance gain is infinitesimal, and is likely overshadowed by the overhead of memory allocation and three system calls in file size computation.

  • And of course, there is no reason to handle one character at a time. It makes it very hard to track the logic. Instead, read the file line by line with fgets, and convert the entire line to an integer with strtol.

\$\endgroup\$
2
\$\begingroup\$

Here are some considerations for best practices:

1. Use Descriptive Variable Names

Using more descriptive variable names can significantly improve the readability of your code. For example, instead of cum, you could use currentSum, and instead of line, currentValue might be more descriptive.

2. Improve Logic for Determining Top 3 Groups

Your logic for updating the top 3 groups could be simplified and made more readable. Consider using an array or a structured data type to hold the top 3 values and sort them when updated.

3. Error Checking

It's good that you're checking for errors when opening and reading the file, but you should also check the return value of fclose and other library functions where applicable.

4. Handling Non-Digit Characters

Your logic for processing each character assumes any non-digit character marks the end of a number. This might not be robust if there are unexpected characters in the file. Consider explicitly checking for whitespace or newline characters to separate numbers.

5. Memory Allocation Error Handling

When malloc fails, it's unnecessary to call handleMemoryError since there's no memory to free. Simply outputting an error message and exiting would suffice.

6. Buffer Overflow Protection

Ensure your logic for converting characters to integers does not lead to integer overflow or buffer overflow vulnerabilities.

Here's a my version of your program incorporating some of these suggestions:

#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>

#define MAX_GROUPS 3

void closeFile(FILE *file) {
    if (file != NULL) fclose(file);
}

void handleFileError(FILE *file, const char *errorMessage) {
    perror(errorMessage);
    closeFile(file);
    exit(EXIT_FAILURE);
}

char* readFile(const char *filename, size_t *size) {
    FILE *file = fopen(filename, "rb");
    if (!file) handleFileError(file, "Error opening file");

    fseek(file, 0, SEEK_END);
    long fileSize = ftell(file);
    fseek(file, 0, SEEK_SET);

    char *buffer = malloc(fileSize + 1);
    if (!buffer) {
        perror("Memory allocation error");
        exit(EXIT_FAILURE);
    }

    *size = fread(buffer, 1, fileSize, file);
    if (*size != fileSize) handleFileError(file, "Error reading file");

    buffer[fileSize] = '\0';
    closeFile(file);
    return buffer;
}

void updateTopGroups(int *topGroups, int value) {
    for (int i = 0; i < MAX_GROUPS; i++) {
        if (value > topGroups[i]) {
            for (int j = MAX_GROUPS - 1; j > i; j--) {
                topGroups[j] = topGroups[j - 1];
            }
            topGroups[i] = value;
            break;
        }
    }
}

int main(int argc, char* argv[]) {
    if (argc < 2) {
        printf("Usage: %s <filename>\n", argv[0]);
        return 1;
    }

    size_t fsize;
    char *buffer = readFile(argv[1], &fsize);

    int topGroups[MAX_GROUPS] = {0};
    int currentValue = 0;
    bool newValue = false;

    for (size_t i = 0; i < fsize; i++) {
        if (buffer[i] >= '0' && buffer[i] <= '9') {
            newValue = true;
            currentValue = currentValue * 10 + (buffer[i] - '0');
        } else if (newValue) {
            updateTopGroups(topGroups, currentValue);
            currentValue = 0;
            newValue = false;
        }
    }
    // Check for a number at the end of the file
    if (newValue) {
        updateTopGroups(topGroups, currentValue);
    }

    printf("Top 3 group sums:\n");
    for (int i = 0; i < MAX_GROUPS; i++) {
        printf("%d\n", topGroups[i]);
    }

    free(buffer);
    return 0;
}

This version introduces a function updateTopGroups to manage the top 3 group sums and uses more descriptive variable names. It also includes a check for the proper usage of the program.

\$\endgroup\$

Your Answer

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

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