Code that finds the largest palindrome from two three-digit factors

Question

My code is really ugly, but I don't know how to make it better.

#include "stdafx.h"
#include <iostream>
#include <math.h>

using namespace std;

bool palindromeTest(double numberVectorAddition){//tests if the factors of the palindrome are both below 3 digits each.
    double p = numberVectorAddition;
    double i = 999;
    double c;
    for (;; i--){
        if (fmod(p, i) != 0)
            continue;
        if (fmod(p, i) == 0)
            c = p / i;
        if (1000 <= c){
            cout << "One of the factors is too high. Looking for next largest palindrome..." << endl;
            cout << endl;
            return false;
            }
        else if (c <= 1000){
        cout << "Factors of the number both below 1000 are " << i << " and " << c;
        return true;
        }
    }
}

void nLargestPalindrome(double number){
    int i = 5; //subtract "1" because array starts at zero
    int intermediate = i; //intermediate value - equal to the array index;
    double *decimalDigit = new double[i + 1];
        while (0 <= i){ //acquire digits of number
            decimalDigit[i] = (number - fmod(number, pow(10, i))) / pow(10, i);
            cout << "Value of digit place " << i << " is " << decimalDigit[i] << endl;
            number = fmod(number, pow(10, i));
            i--;
        }
        for (int q = 0; q < 100; q++){ //decreases size of palindrome, and also tests after the decrease.
            double numberVectorAddition = pow(10, 5)*decimalDigit[0] + pow(10, 4)*decimalDigit[1] + pow(10, 3)*decimalDigit[2] + pow(10, 2)*decimalDigit[3] + pow(10, 1)*decimalDigit[4] + decimalDigit[5];
            cout << decimalDigit[0] << decimalDigit[1] << decimalDigit[2] << decimalDigit[3] << decimalDigit[4] << decimalDigit[5] << '\t';
            cout << numberVectorAddition << endl;
            if (palindromeTest(numberVectorAddition))
                break;
            decimalDigit[2]--;
            decimalDigit[3]--;
                if (decimalDigit[2] == 0){
                    numberVectorAddition = pow(10, 5)*decimalDigit[0] + pow(10, 4)*decimalDigit[1] + pow(10, 3)*decimalDigit[2] + pow(10, 2)*decimalDigit[3] + pow(10, 1)*decimalDigit[4] + decimalDigit[5];
                    cout << decimalDigit[0] << decimalDigit[1] << decimalDigit[2] << decimalDigit[3] << decimalDigit[4] << decimalDigit[5] << '\t';
                    cout << numberVectorAddition << endl;
                    if(palindromeTest(numberVectorAddition))
                        break;
                        if ((decimalDigit[1] == 0) && (decimalDigit[2] == 0)){
                            decimalDigit[1] = 9;
                            decimalDigit[4] = 9;
                            decimalDigit[2] = 9;
                            decimalDigit[3] = 9;
                            decimalDigit[0]--;
                            decimalDigit[5]--;
                            continue;
                        }
                    decimalDigit[2] = 9;
                    decimalDigit[3] = 9;
                    decimalDigit[1]--;
                    decimalDigit[4]--;
                }
        }
    delete decimalDigit;
}


int main(){
    double n = 997799; //997799 is equal to 999*999 - 2; palindrome is smaller.
    nLargestPalindrome(n); //finds the largest palindrome below "n" that has both factors of three digits.
    cin.get(); //stops the command window from closing prematurely
}

Answer 1

Code formatting

Your code seems to be poorly indented. I suggest you have a look at the documentation of your favorite text editor to see how to fix this.

Basic logic

It doesn't make much sense to check :

    if (fmod(p, i) != 0)
        continue;
    if (fmod(p, i) == 0)

Similarly,

        if (1000 <= c){
            return false;
        }
        else if (c <= 1000){

is quite redundant.

using namespace std;

Using using namespace std; is usually frowned upon, some will say that it is ok in a cpp file, some will say the opposite. I'll let you decide.

The right type

You are using double but long int are more than enough for what you are trying to do.

Right place to define variables

It is usually a good idea to define variables in the smallest possible scope. Among other things, it helps the reader not to have to keep too many things in mind. Also, it helps you see that intermediate is an unused variable. As a side-note, it is a good idea to activate all the warnings in your compiler to detect such a thing (and many other potential errors).

Define constants

Instead of having 5 in the middle of your code and need to comment it, you could store 6 in a constant named for instance NB_DIGITS and remove the need for any comment.

Use for loops when you can

Now, the loop to aquire digits can be rewritten :

for (int i = NB_DIGITS -1; 0 <= i; i--) { //acquire digits of number
    decimalDigit[i] = (number - fmod(number, pow(10, i))) / pow(10, i);
    cout << "Value of digit place " << i << " is " << decimalDigit[i] << endl;
    number = fmod(number, pow(10, i));
}

Compute things only once

You call pow(10, i) way too many times compared to what you actually need. You can simply write :

    double pow_ten = pow(10, i);
    decimalDigit[i] = (number - fmod(number, pow_ten)) / pow_ten;
    cout << "Value of digit place " << i << " is " << decimalDigit[i] << endl;
    number = fmod(number, pow_ten);

Smarter decomposition of a number

Instead of going through the hassle of getting digits starting with the one with the strongest weight, it is much easier to start by the end. Also, this makes you perform the iteration in the logicial order.

Better names

numberVectorAddition is a pretty bad name for a parameter. n conveys enough information as far as I can tell.

palindromeTests is also quite bad. hasDivisorsInRange is probably better.

Finally, DecimalDigit is a bit redundant are digits already include the notion of 10 but also, it probably should be plural to tell that it's a collection.

No need for new/delete here

You don't need to use new/delete here. If you just define it long int digits[NB_DIGITS];, it will be deleted when it does out of the scope.

Clearer responsability for functions

It could be interesting for your function to return values instead of printing them. This would lead to separation of concerns. One of the multiple advantages is to make your code easier to test.

At this point your code, looks like :

#include <iostream>
#include <math.h>

using namespace std;

const int NB_DIGITS = 6;

bool hasDivisorsInRange(long int n){//tests if the factors of the palindrome are both below 3 digits each.
    for (long int i = 999;; i--){
        if (n % i == 0)
        {
            long int c = n / i;
            if (1000 <= c){
                return false;
            }
            else if (c <= 1000){
                cout << "Factors of the number both below 1000 are " << i << " and " << c << endl;
                return true;
            }
        }
    }
}

long int LargestPalindrome(long int number){
    long int digits[NB_DIGITS];
    for (int i = 0; i < NB_DIGITS; i++) { //acquire digits of number
        digits[i] = number % 10;
        number /= 10;
        cout << "Value of digit place " << i << " is " << digits[i] << endl;
    }
    for (int q = 0; q < 100; q++){ //decreases size of palindrome, and also tests after the decrease.
        long int n = pow(10, 5)*digits[0] + pow(10, 4)*digits[1] + pow(10, 3)*digits[2] + pow(10, 2)*digits[3] + pow(10, 1)*digits[4] + digits[5];
        cout << digits[0] << digits[1] << digits[2] << digits[3] << digits[4] << digits[5] << '\t';
        cout << n << endl;
        if (hasDivisorsInRange(n))
            return n;
        digits[2]--;
        digits[3]--;
        if (digits[2] == 0){
            long int n2 = pow(10, 5)*digits[0] + pow(10, 4)*digits[1] + pow(10, 3)*digits[2] + pow(10, 2)*digits[3] + pow(10, 1)*digits[4] + digits[5];
            cout << digits[0] << digits[1] << digits[2] << digits[3] << digits[4] << digits[5] << '\t';
            cout << n2 << endl;
            if(hasDivisorsInRange(n2))
                return n;
            if ((digits[1] == 0) && (digits[2] == 0)){
                digits[1] = 9;
                digits[4] = 9;
                digits[2] = 9;
                digits[3] = 9;
                digits[0]--;
                digits[5]--;
                continue;
            }
            digits[2] = 9;
            digits[3] = 9;
            digits[1]--;
            digits[4]--;
        }
    }
    return 0;
}


int main(){
    long int n = 906608; //997799 is equal to 999*999 - 2; palindrome is smaller.
    int pal = LargestPalindrome(n); //finds the largest palindrome below "n" that has both factors of three digits.
    cout << "Solution is " << pal << endl;
}

Better algorithm

You are trying to iterate over palindromes in a quite complicated way (that I do not understand at all). To iterate over palindromes composed of 6 digits is nothing but iterating over values between 100 and 999 and adding the mirrored version of the number at the end.

This can be badly written :

long int LargestPalindrome(long int number){
    for (int beg = (number / 1000); beg > 100; beg--)
    {
        // Could be done in a clean way with array but I'm lazy
        int i = beg;
        int a = i % 10; i/=10;
        int b = i % 10; i/=10;
        int c = i % 10; i/=10;
        long int pal = beg * 1000 + 100 * a + 10 * b + c;
        if (pal < number && hasDivisorsInRange(pal))
            return pal;
    }
    return 0;
}

For n = 906608, this finds 888888 which happens to be a better solution that the one found by your code.

Also, you could perform a smarter research for divisors.

I have to go, I'll try to continue this answer at some point.