Project Euler 14: Longest Collatz Sequence

I've brute forced solve Project Euler 14. This is my first attempt at trying to solve it, by caching already solved answers, and skipping them as I run it, I am finding the program take's almost twice as long as the simple brute force method of checking each number.

It works, but I could also use some insight on how to stop calculating, once I run into an integer we have already solved, and how to better write the entire problem into a class.

/*Project Euler 14.
The following iterative sequence is defined for the set of positive integers
n → n/2 (n is even)
n → 3n + 1 (n is odd)
Using the rule above and starting with 13, we generate the following sequence:
13 → 40 → 20 → 10 → 5 → 16 → 8 → 4 → 2 → 1
It can be seen that this sequence (starting at 13 and finishing at 1) contains 10 terms. Although it has not been proved yet (Collatz Problem), it is thought that all starting numbers finish at 1.
Which starting number, under one million, produces the longest chain?
NOTE: Once the chain starts the terms are allowed to go above one million
*/

#include <iostream>
#include <vector>

//I use this class to mark if a number has been solved.

class Collatz{
public:
bool solved = false;
unsigned terms = 0;
};

int main (){

const unsigned maxnum = 1000000;
std::vector<Collatz>cz(maxnum);
unsigned larger = 0; //will be used to find largest sequence.
unsigned largest = 0;

//This loop checks to see if the num has been solved yet.
//If not it solves its sequence, and marks all numbers in the sequence.

for(unsigned a = 2; a < maxnum; a++){
if(cz[a].solved == false){
std::vector<unsigned long long>cache;
cache.push_back(a);
for(unsigned long long n = a; n != 1;){
if(n % 2 == 0)
n = n/2;
else
n = 3*n+1;
cache.push_back(n);
}
//caching all already sovlved numbers
for(unsigned c = 0; c < cache.size(); c++){
if(cache[c] <= maxnum){
cz[cache[c]].solved = true;
cz[cache[c]].terms = cache.size() - c;
}
}
}
//Storing largest term.
if(larger < cz[a].terms){
larger = cz[a].terms;
largest = a;
}
}

std::cout << largest << std::endl;
}


An obvious improvement is to check n for being already solved on each iteration of for n loop. As coded, you still go all the way to 1, which might be quite long. Something along the lines of

for ( a = 2; a < maxnum; ++a) {
std::vector<unsigned long long>cache;
for (n = a; n != 1 && !cz[n].solved; ) {
cache.push_back(n);
n = (n%2 == 0)? n/2: 3*n+1;
}
// make cached data solved
...
}


If cz[1].solved is preset to true, the loop condition can be simplified to

for (n = a; !cz[n].solved; )

• I have to make sure I add the terms, to get the correct terms. Commented Apr 13, 2014 at 21:21

Try adding the following:

#include <limits>
#include <sstream>
#include <stdexcept>

int main() {
constexpr unsigned max_odd = (std::numeric_limits<unsigned>::max() - 1) / 3;

…
for … {
if … {
…
for … {
if(n % 2 == 0) {
n = n/2;
} else if (n > max_odd) {
std::stringstream ss;
ss << n;
throw std::out_of_range(ss.str());
} else {
n = 3*n+1;
}
…


… and I think you'll find your problem.

unsigned is actually severely undersized: it is only guaranteed to hold values up to 65535.

Incidentally, that is an illustration of why you should always include braces with your if- and else-clauses. Having braces on only the else if clause that I just added would be weird and gross. On the other hand, I shouldn't have to retrofit the existing if and else with braces just to add my else if — that would create uglier diffs in version control.

• Wouldn't the program give the wrong answer if that was the case? Commented Apr 13, 2014 at 18:43
• It might give a wrong answer, or it might end up in an infinite loop (seemingly disproving the Collatz conjecture). Commented Apr 13, 2014 at 19:08
• I am getting the correct answer though, so I don't think that is the case. Commented Apr 13, 2014 at 19:38