Since everyone's posting competing versions of the final code, after refactoring I end up with
public static int GreatestPalindrome(int minNumber, int maxNumber)
{
long biggestNumber = 0;
for (int left = maxNumber; left >= minNumber; left--)
{
for (int right = maxNumber; right >= left; right--)
{
var test = left * right;
if (test < biggestNumber) break;
if (IsPalindrome(test)) biggestNumber = test;
}
}
return biggestNumber;
}
called as GreatestPalindrome(100, 999). The body of the if (IsPalindrome(test)) block could break;, but the next iteration of the loop will do that anyway. If I were really interested in microoptimising I'd expland the scope of test and replace the multiplication with a subtraction, but pace the impression some people seem to have, microoptimising wasn't the goal of my review.
Since everyone's posting competing versions of the final code, after refactoring I end up with
public static int GreatestPalindrome(int minNumber, int maxNumber)
{
long biggestNumber = 0;
for (int left = maxNumber; left >= minNumber; left--)
{
for (int right = maxNumber; right >= left; right--)
{
var test = left * right;
if (test < biggestNumber) break;
if (IsPalindrome(test)) biggestNumber = test;
}
}
return biggestNumber;
}
called as GreatestPalindrome(100, 999). The body of the if (IsPalindrome(test)) block could break;, but the next iteration of the loop will do that anyway. If I were really interested in microoptimising I'd expland the scope of test and replace the multiplication with a subtraction, but pace the impression some people seem to have, microoptimising wasn't the goal of my review.
I'm going to guess that you're a C programmer by background, because of the way you've declared all of the variables at the top of the method. That's considered bad practice in most modern languages, because it obscures the scope of the variables. So the first refactor I would recommend is to declare the variables where they're first used, or in the enclosing scope if the first use is in too narrow a scope:
int biggestNumber = 0;
string returnString=string.Empty;
for (int left=maxNumber; left >= 0; left--)
{
for(int right=maxNumber; right >= 0; right--)
{
int test = left * right;
...
There's a lot of unnecessary searching. Consider the additional constraints we can impose without affecting correctness*:
left >= 100andright >= 100(as suggested in Velial's answer)right >= left(since multiplication is symmetric)right >= biggestNumber / left(since otherwisetest < biggestNumber)
* Well, see next point
Ok, there is one issue with respect to affecting correctness.
for(right=maxNumber; right >= 0; right--)
{
test = left * right;
...
breakOut = ...;
if (breakOut )
{
break;
}
}
if (test>biggestNumber)
{
biggestNumber = test;
returnString = $"Palindrome: {test}, Left: {left}, Right: {right}";
}
The correctness currently relies on the fact that if we don't break then when we hit the if (test>biggestNumber) we have test == 0 because of the loop termination condition. That's rather convoluted.
The logic for breaking out of the loop, which should really be explained in a comment somewhere, is that the largest palindrome over all (left, right) is at least as large as the largest palindrome for a given left, so having found that largest palindrome we don't need to consider the rest. So a better name (maybe not the best name) for breakOut would be testIsPalindrome, and then it becomes obvious that the if (test>biggestNumber) block should be inside the if (testIsPalindrome) block.
string testNumberAsString = Convert.ToString(test);
string reverse = string.Empty;
for (int index = testNumberAsString.Length; index > 0; index--)
{
reverse += testNumberAsString[index-1];
}
It's debatable whether you should convert to a string to test for being a palindrome or just to a list of digits, but that's a minor issue. The bigger issue is that in C# strings are immutable, which means that += really allocates a new block of memory, copies across the lval's characters, and then copies across the rval. Rule of thumb: never use string + anything in a loop. Instead use the mutable string class System.Text.StringBuilder.
Incidentally, reversing a string is a lot more subtle than most people realise.