UVA 100: “The 3n + 1 problem” take 2

This is my second attempt to solve this problem, my first attempt is here: UVA 100: "The 3n + 1 problem"

This time I tried to take lessons from the failures and mistakes You pointed out in my first attempt.

• I resigned from packing multiple statements in one line. (This resulted in the code length balooning, though :( )

• I removed constructs that I had placed to “optimize” the code, but that were shown to be slowing it down instead, that is std::pow, std::log2 and most importantly, std::unordered_map.

• I did some actual benchmarking, and I think the code is reasonably fast now.

• I added comments whenever I thought my intent wasn’t obvious.

• However I fail to understand why, but OK, I’ve put away the using namespace std

• I still tried to make the code error proof, for example by checking input correctness.

• I’ve put assert macros.

Any comments for this code?

```
// This program solves UVA Online Judge Problem 100: "The 3n + 1 Problem"
// Problem specification:
// https://uva.onlinejudge.org/index.php?option=com_onlinejudge&Itemid=8&page=show_problem&problem=36

#ifdef ONLINE_JUDGE
#define NDEBUG
#endif

#include
#include
#include
#include
#include
#include
#include
#include
#include
#include

// aoo = At Or Operator[]
template
typename Container::reference
aoo(Container &cont, typename Container::size_type index) {
#ifndef NDEBUG
return cont.operator[](index);
#else
return cont.at(index);
#endif
}

template
typename Container::value_type::reference
aoo(Container &cont, typename Container::size_type i1,
typename Container::value_type::size_type i2) {
#ifndef NDEBUG
return cont.operator[](i1).operator[](i2);
#else
return cont.at(i1).at(i2);
#endif
}

using seqlen = std::uint_fast16_t;
using seqval = std::uint_fast32_t;

// As of now, the problem specification incorrectly states that all integers
// in the input will be less than 10.000. The correct boundaries, stating that
// integers will be less t

-
A lot of variables have extremely weird names (like aoo. What does that even mean? "at or operator()", seriously?).

-
You define cache_t to be a vector of vectors and then create a bunch of huge unreadable functions to work with it (including initialization). The get_length_from_cache is really huge. I have no idea what's going on in there (and what's the point of creating a lot of lambda functions inside it? It seems to me that it only increases the confusion). Everything related to cache_t looks like a bunch of mess to me. I'd strongly recommend to create a properly documented class with a meaningful name that implements your cache with short, readable, properly named methods inside it. Another argument for making it a separate class is that it's not really a vector of vectors: it's some kind of data structure (I can't figure out what's going on exactly in your code) that uses a vector of vectors internally.

-
Adding the comments doesn't magically make your code more readable. Ideally, the code should be self-documenting. It's definitely not the case here. As I have said before, I have no idea what the cache does.

-
I don't see that point of having the test_case struct and doing all that complicated IO stuff. The problem statement guarantees that the input is correct. It could be just something like:

cin >> i >> j;
cout << get_length_from_cache(...) << '\n';

-
If you want to make the code readable, you should:

• structure it properly (if something is a separate entity, like you cache, it should be implemented as a separate class)

• Avoid making the code complex whenever possible. A simpler way to do the same thing is usually a better one.

StackExchange Code Review Q#158092, answer score: 3