4
\$\begingroup\$

Part 1:

The task involves analyzing a calibration document containing lines of text. Each line represents a calibration value that needs to be recovered by extracting the first and last digits and combining them into a two-digit number. The goal is to find the sum of all these calibration values.

For example:

1abc2
pqr3stu8vwx
a1b2c3d4e5f
treb7uchet

In this example, the calibration values of these four lines are 12, 38, 15, and 77. Adding these together produces 142.

#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>
#include <string.h>
#include <errno.h>

/* This would more than just suffice for the sample data. */
#define BUFSIZE 1024

static size_t calibration_value(const char *buf)
{
    size_t start = 0;
    size_t end = 0;

    /* Note: This would return true for 0, but 0 is not part of the 
    * problem specification, or the input, so we don't need to check
    * for it.
    */
    for (size_t i = 0; buf[i]; ++i) {
        if (isdigit((unsigned char) buf[i])) {
            start = !start ? buf[i] - '0' : start;
            end = buf[i] - '0';
        }
    }

    return start * 10 + end;
}    

static size_t calibration_total(FILE *stream) 
{
    char buf[BUFSIZE];
    size_t total = 0;
    
    while (fgets(buf, sizeof buf, stream)) {
        total += calibration_value(buf);
    }
    return total;
}

int main(int argc, char **argv)
{
    if (!argv[0]) {
        fputs("Fatal - A null argv[0] was passed in.", stderr);
        return EXIT_FAILURE;
    }

    if (argc != 2) {
        fprintf(stderr, "Error - No file provided.\n"
              "Usage: %s <FILE>\n", argv[0]);
        return EXIT_FAILURE;
    }

    FILE *const fname = (errno = 0, fopen(argv[1], "r"));

    if (!fname) {
        errno ? perror(argv[1]) : (void) fputs("Error - Failed to open file.", stderr);
        return EXIT_FAILURE;
    }

    printf("%zu\n", calibration_total(fname));
    return EXIT_SUCCESS;
}

Part 2:

In part two, the calibration document may include digits spelled out with letters (e.g., "one," "two," etc.). The task is to identify the actual first and last digits in each line, taking into account both numerical digits and spelled-out numbers. The overall objective remains the same: find the sum of the calibration values obtained from combining the first and last digits of each line.

For example:

two1nine
eightwothree
abcone2threexyz
xtwone3four
4nineeightseven2
zoneight234
7pqrstsixteen

In this example, the calibration values are 29, 83, 13, 24, 42, 14, and 76. Adding these together produces 281.

#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>
#include <string.h>
#include <errno.h>

#define INIT_DMAP(_name, _val) {.dname = _name, .val = _val}

struct digit_map {
    const char *const dname;
    const unsigned int val;
} digits[] = {
    INIT_DMAP("one", 1),
    INIT_DMAP("two", 2),
    INIT_DMAP("three", 3),
    INIT_DMAP("four", 4),
    INIT_DMAP("five", 5),
    INIT_DMAP("six", 6),
    INIT_DMAP("seven", 7),
    INIT_DMAP("eight", 8),
    INIT_DMAP("nine", 9)
};

/* This would more than just suffice for the sample data. */
#define BUFSIZE 1024

static size_t calibration_value(const char *buf)
{
    unsigned int start = 0;
    unsigned int end = 0;

    for (size_t i = 0; buf[i]; ++i) {
        /* Note: This would return true for 0, but 0 is not part of the 
         * problem specification, or the input, so we don't need to check
         * for it.
         */
        if (isdigit((unsigned char) buf[i])) {
            start = !start ? buf[i] - '0' : start;
            end = buf[i] - '0';
        } else {
            for (size_t j = 0; j < sizeof digits / sizeof *digits; ++j) {
                if (!strncmp(buf + i, digits[j].dname, strlen(digits[j].dname))) {
                    start = !start ? digits[j].val : start;
                    end = digits[j].val;
                }
            }
        }
    }
    
    return start * 10 + end;
}

static size_t calibration_total(FILE *stream)
{
    char buf[BUFSIZE];
    size_t total = 0;

    while (fgets(buf, sizeof buf, stream)) {
        total += calibration_value(buf);
    }
    
    return total;
}

int main(int argc, char **argv)
{
    if (!argv[0]) {
        fputs("Fatal - A null argv[0] was passed in.", stderr);
        return EXIT_FAILURE;
    }

    if (argc != 2) {
        fprintf(stderr, "Error - No file provided.\n"
                "Usage: %s <FILE>\n", argv[0]);
        return EXIT_FAILURE;
    }

    FILE *const fname = (errno = 0, fopen(argv[1], "r"));

    if (!fname) {
        errno ? perror(argv[1]) : (void) fputs("Error - Failed to open file.", stderr);
        return EXIT_FAILURE;
    }

    printf("Total: %zu\n", calibration_total(fname));
    return EXIT_SUCCESS;
}

What improvements can be made to the code?

Note: 0/zero isn't part of the input, nor is included in the problem specification, so it is not a valid digit.

\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

Don't assume all lines will fit into a fixed-size buffer

You use a buffer with a hardcoded size of 1024 characters. As you write in the comments, it suffices for the sample data. But what if you would use this code on a different input that could have arbitrarily long lines?

The problem is not so much the size of the buffer, but rather that you assume each and every line in the input will fit into that buffer. I see two possible ways to make it more robust:

  1. Use the POSIX getline() function. It will allocate a large enough buffer for you.
  2. Use the fixed size buffer, but check if it actually contains the newline character '\n'. If not, keep the intermediate start and end value,fgets() again, and then continue updating start and end, until you actually do see the end of the line.

The former option is easy, but might use a lot of memory. The second option is a little bit more work but will work even if you have very little memory.

Inappropriate use of size_t

size_t should be used for sizes, counts and indices. So it's the right type to use for the loop counter i for example. However, calibrarion_value() can only return a value of at most 99, so size_t is complete overkill here. I recommend just using unsigned int here as it's large enough, and usually the fastest for the CPU to work with as well.

On the other hand, the input file might actually be larger than would fit into memory, and size_t might be too small for total. It's better to use a fixed-size integer type: either you know the largest value you expect and you can choose the proper type accordingly, or just use uintmax_t.

Avoid comma and ternary operator tricks

While it might be "smart" and result in a bit more compact code, avoid using tricks using the comma operator or ternary operator. Just write multiple statements and regular if-statements:

errno = 0;
FILE *const fname = fopen(argv[1], "r");

if (!fname) {
    if (errno) {
        perror(argv[1]);
    } else {
        fputs("Error - Failed to open file.", stderr);
    }
    return EXIT_FAILURE;
}

While it is more verbose, it is easier to understand, meaning the code will be more maintainable and will have less chance of bugs.

Naming

If I see a variable named fname, I might think that this is a string holding a filename. However, it is a pointer to the FILE object itself. I would rather call it file.

Missing error handling

While you check for errors when opening a file, you don't check for errors while reading from it. I/O errors can happen at any point. Check that feof(stream) is true at the end of calibration_total(); if it's not then you know something happened before getting to the end of the file. Print an error message and return EXIT_FAILURE in this case.

INIT_DMAP is unnecessary

You don't need this macro. Simply write:

struct digit_map {
    const char *const dname;
    const unsigned int val;
} digits[] = {
    {"one", 1},
    {"two", 2},
    …
};

The order of the member variables is right there at the top, so it's hard to make a mistake here.

Performance

In part two, you use a very simple algorithm to find digits spelled out with letters. However, it means that for almost every non-numeric character, you use 10 calls to strncmp(). A better solution might be to use a prefix tree.

Also, once you have found a word describing a digit, then you can also know how many characters you can skip. For example, if you found "seven", you know that you can skip the next three characters ("eve"), and continue from the "n" (as that might become a "nine").

However, if you are interested in improving performance, don't blindly implement a different algorithm, actually measure the performance of all the alternatives on some representative input.

\$\endgroup\$
6
  • \$\begingroup\$ Would if (ferror(file) || !feof(file) { /* Error */ be more apt? \$\endgroup\$
    – Harith
    Commented Dec 9, 2023 at 10:43
  • \$\begingroup\$ Part 2 of the 'fixed-size buffer' works well until one part of the digit, for example, 'sev,' is in one line, and 'en' is in the next. Perhaps I should continue reading into a larger buffer until encountering a newline, then check for the first and last digits. Do you have a simpler alternative? (Excluding the use of getline()) \$\endgroup\$
    – Harith
    Commented Dec 9, 2023 at 10:56
  • 1
    \$\begingroup\$ For part two, if you read a partial line, just scan until 5 characters before the end of the buffer. Then move the last 5 characters to the start of the buffer, and fgets() into the rest of the buffer after that, and then restart scanning from the beginning of the buffer. \$\endgroup\$
    – G. Sliepen
    Commented Dec 9, 2023 at 11:57
  • 1
    \$\begingroup\$ If you want that approach reviewed, feel free to create a new question here on Code Review. \$\endgroup\$
    – G. Sliepen
    Commented Dec 10, 2023 at 15:17
  • 1
    \$\begingroup\$ Yes, I have done so here: codereview.stackexchange.com/questions/288373/… \$\endgroup\$
    – Harith
    Commented Dec 12, 2023 at 18:46

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.