Skip to main content
deleted 1 character in body
Source Link
Peter Taylor
  • 24.2k
  • 1
  • 48
  • 93

Linear search is never ideal. If you're using a recent standard, std::unordered_map is asymptotically preferable, and also IMO conveys the purpose better. Or maybe you would be better off with sortstd::map, which would remove the need to sort at the end.

Linear search is never ideal. If you're using a recent standard, std::unordered_map is asymptotically preferable, and also IMO conveys the purpose better. Or maybe you would be better off with sort::map, which would remove the need to sort at the end.

Linear search is never ideal. If you're using a recent standard, std::unordered_map is asymptotically preferable, and also IMO conveys the purpose better. Or maybe you would be better off with std::map, which would remove the need to sort at the end.

Source Link
Peter Taylor
  • 24.2k
  • 1
  • 48
  • 93

The basic approach looks sensible, but I do have a few suggestions for improvements.

using namespace std;

Bad idea. It may be a nuisance having to prepend std:: everywhere, but it's a short name in order to reduce the nuisance, and the things which could go wrong are quite bad.


void add_base_and_power(Prime_factorization& prime_factorization, int    base, int power)

What happened to the whitespace in int base? A single space suffices.

What do the names communicate and fail to communicate? The name prime_factorization communicates nothing: the type already says that. What purpose does this particular prime factorisation serve? The name base doesn't really fit the context IMO: you're not accumulating to a Base_power_pairs. add_prime_power(Prime_factorization accumulator, int prime, int power) would make it crystal clear what the function does.


void check_if_is_factor(Prime_factorization& prime_factorization, int &number, int divisor)

Again, names.

    if (number % divisor == 0) { // If the divisor is a base, we divide until it no longer can

This isn't wrong, but I wonder whether you made a conscious choice between this approach and

    int power { 0 };
    while (number % divisor == 0) {
        number /= divisor;
        ++power;
    }

    if (power > 0)
    {
        // Add base and power to factorization
        add_base_and_power(prime_factorization, divisor, power);
    }

I find the latter more aesthetically pleasing. It doesn't do a test which it knows will succeed.

NB this point is purely a subjective point about style - I'm not trying to microoptimise, and microoptimisations should be done with good benchmarks.


void decompose(Prime_factorization& prime_factorization, int number)

What is being decomposed here? My instinct would be that the first argument is the most relevant to interpreting the function name, but that doesn't make sense in this case.

    for (int div { 3 }; div * div <= number && number > 1; div += 2)

I've seen you use this int name { val } a few times, and (not being a C++ user) I'm not familiar with it. Does it have some advantage over int name = val to counter the disadvantage of requiring more effort to port to a different language?

There's a case to be made for calculating the square root of number outside the loop: firstly on micro-optimisation grounds, but secondly on correctness grounds. div * div could overflow the type.


    // We must print the base-power combinations in sorted order
    sort(prime_factorization.begin(), prime_factorization.end(), [](pair<int, int> p1, pair<int, int> p2) { return p1.first < p2.first; });

    if (okay) {
        cout << 1 << '\n';

        for (auto& i : prime_factorization)
            cout << i.first << ' ' << (i.second / m) << '\n';
    } else
        cout << 0;

Two things here: firstly, since the sorted values are only needed in one branch of the if it makes more sense to push the sort into that branch. Secondly,

if (test) {
    foo;
} else
    bar;

is easily the weirdest bracing style I've ever seen, and weird style is not good for collaboration. Do you use a company or published style guide?


Finally, my one algorithmic observation.

    // Check if we've already got the base we're trying to add
    auto it = find_if(prime_factorization.begin(), prime_factorization.end(), [&](pair <int, int> p) { return p.first == base; } );

    if (it != prime_factorization.end()) // If yes, add the power to the existing base
        it->second += power;
    else
        prime_factorization.push_back(make_pair(base, power)); // Else, create a new base

Linear search is never ideal. If you're using a recent standard, std::unordered_map is asymptotically preferable, and also IMO conveys the purpose better. Or maybe you would be better off with sort::map, which would remove the need to sort at the end.