4
\$\begingroup\$

Task:

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.

Code:

#include <cstdlib>
#include <fstream>
#include <iostream>
#include <system_error>

static int calibration_total(std::ifstream &in_file) noexcept 
{
    const std::string accept{"0123456789"};
    int sum{};
    std::string line{};

    /* This is not very pedantic, we are assuming that the methods
     * would never return `npos`.
     */
    while (std::getline(in_file, line)) {
        int first_digit{line[line.find_first_of(accept)] - '0'};
        int last_digit{line[line.find_last_of(accept)] - '0'};
        sum += first_digit * 10 + last_digit; 
    }

    return sum;
}

int main(int argc, char *argv[]) 
{
    if (argc != 2) {
        std::cerr << "Usage: " << (argv[0] ? argv[0] : "") << " <filename>\n";
        return EXIT_FAILURE;
    }

    std::ifstream in_file{argv[1]};

    if (!in_file.is_open()) {
        std::cerr << "Error: failed to open " << argv[1] << " - "
                  << std::error_code{errno, std::generic_category()}.message()
                  << ".\n";
        return EXIT_FAILURE;
    }

    const int sum{calibration_total(in_file)};

    if (!in_file.eof()) {
        std::cerr << "Error: failed to read input file " << argv[1] << " - "
                  << std::error_code{errno, std::generic_category()}.message()
                  << ".\n";
        return EXIT_FAILURE;
    }

    std::cout << sum << "\n";
    in_file.close();
    return EXIT_SUCCESS;
}

Review Request:

General coding comments, style, bad/outdated practices et cetera.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Reminder, do not modify the code after someone has posted an answer. \$\endgroup\$
    – pacmaninbw
    Feb 14 at 14:14

3 Answers 3

7
\$\begingroup\$

This looks very good, you have paid attention to many details. I especially appreciate the correct way to handle input errors. Still, a few things can be improved:

Remove noexcept

std::getline() is not noexcept, so it is possible that exceptions will be thrown, for example if it needs to resize line but you are out of memory. By adding noexcept you now prevent the caller from handling any exceptions thrown inside calibration_total().

There is also no benefit whatsoever from making this function noexcept. I recommend that you avoid adding noexcept to things, unless you really know that it might give a performance benefit.

Efficiency

The call to find_last_of() starts from the beginning of the line, but you already know where the first digit is. You can use the result of find_first_of() and only pass a substring starting at that position to find_last_of(), that avoids doing some work twice. Of course, making a substring is expensive in itself, but you can avoid that by using std::string_views.

It might be faster to use std::ranges::find() and std::ranges::find_end() with std::isdigit() as the predicate, as that avoids scanning the string accept for every character in line.

Print something useful if !argv[0]

If argv[0] is nullptr, you print an empty string for the program name. That's not very nice, it will then print:

Usage:  <filename>

I would just print the most likely name the binary will have.

Unnecessary statements

The statement in_file.close() is unnecessary; a std::ifstream object will close itself automatically when its scope ends.

You also don't need return EXIT_SUCCESS at the end of main(), although it's not wrong.

Finally, the test for in_file.is_open() is also not really necessary; if it wasn't opened, then calibration_total() will immediately exit, and in_file.eof() will still be false, so you are still printing an error message in that case.

\$\endgroup\$
0
7
\$\begingroup\$

I'll only add things not brought up in previous reviews yet. Generally, the code is well readable and structured, I'd enjoy working with it!

Use Of static Functions

Not an important thing, but typically in C++ purely local functions are put into an anonymous namespace. I'm not even sure if there's a semantic difference between the two, but it surely is negligible.

ifstream vs istream

Your calibration_total() function only uses the base istream interface, no need to provide a reference to a ifstream. It would allow writing unit tests based on stringstream, too.

Undefined Behaviour With Malformed Input

If a line contains no digits, find_* will not return a valid index. Reading at that index causes UB. In addition, if the input contains just a single digit, the result will probably be wrong, i.e. you get silent data corruption.

You comment the code with "we are assuming that the methods would never return npos", so you seem to be aware of it. I don't think this assumption is a good one though, because it is completely outside of your code's control.

Naming

This is a controversial topic, just for your info. Now, about calibration_total(), it lacks a verb. calc or compute as a suffix would make it clearer to me.

\$\endgroup\$
1
  • 3
    \$\begingroup\$ There's no semantic difference between static and anonymous namespace. static for file local functions was deprecated but then un-deprecated because it was just never going to be removed; however anonymous namespace is generally preferred because it removes one of the many overloaded meanings for static. \$\endgroup\$ Feb 14 at 18:17
3
\$\begingroup\$

I would recommend splitting into more functions, have the three lines in the while inside calibration_total could be in a different function, so in my opinion it may be more readable so you have a function that just read from the file and feeds it to a different function. also most of mains error handling could be in a designated function. I solved this question when it came out I won't spoil part 2 but this approach may completely fail on part 2, although I didn't given it a lot of thought maybe it is modifiable and could work for part 2.

in general my approach is splitting into as many functions as possible sometimes it may come at the cost of performance for example I always like to parse into a say a vector my input so here I stored the numbers in a vector and then summed them up.

good luck, advent of code is a very fun and a good learning experience!

\$\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.