# ProjectEuler problem 4. Feedback request

BE AWARE: this is working solution. If you want to solve it on your own, consider not to read the code.

Discussions related to implementation:
Optimization

Thanks to all who answered my questions, feel like I learned much when solving relatively easy problem.

I am using comments like I saw in MIT problem sets plus some improvisation. Course “Introduction to programming using Python”. Such problem sets are on MIT open course ware, I am sure anybody who wants can find it(I am not sure if link to such resource is appropriate for this forum).

Are there any conventional rules in JS in terms of commenting I am not using and should use?

Implementation:

``````function isNumberPalindromic(num) {
/*function was originally implemented to work with strings
was optimized to only work with numbers:
was renamed from palindrome() to isNumberPalindromic()
part of code for formatting strings(regexp) was replaced
Expected input:
num: natural number
Expected output:
true if number is palindromic
false otherwise
*/

let formattedNum = ""+num;
if (formattedNum=='' || formattedNum.length == 1) {
return true;
}
else {
return formattedNum[0] == formattedNum[formattedNum.length - 1] &&
isNumberPalindromic(formattedNum.slice(1,-1));
}
}

/*Unit testing
testing environment: onecompiler.com
console.log(isNumberPalindromic(9009));//true
console.log(isNumberPalindromic(9116));//false
console.log(isNumberPalindromic(9345));//false
*/

function largestPalindromeProduct(n) {
/*
Expected input:
n: natural number > 0, number of digits
Expected output:
number, the largest palindromic product of 2 n-digit numbers
*/

let result = 0;
for (let i = Math.pow(10, (n - 1)); i < Math.pow(10, n); i++) {
// if i == 2 >>> start step i = 10; end step i < 100

for (let j = i; j < Math.pow(10, n); j++) {
// j = i >>> exploiting math pattern to reduce number of checks
// TODO: try to estimate big O notation for these nested loops for the sake of learning

let tempResult = i * j; /*TODO:
figure out what's best: use variable or end up with more multiplications*/

if (tempResult > result && isNumberPalindromic(tempResult)) {
result = tempResult;
}
}
}

return result;
}

//all tests passed at fCC
``````

It is always about function and variable names with me. Since your function is named `isNumber Palindromic` I do not see any reason to have the above comment. It is obvious that the return value is Boolean. This in just my opinion.

Just to clarify, do you mean integers or natural numbers? If integers that means it would handle negative ones too. If truly only natural numbers, then you could name your parameter `naturalNum` and get rid of this comment.

Unless you are changing the value of `formattedNum` somewhere else in the function, stick with `const` instead of `let`. Also, if you want to be more explicit, then instead of coercing the number into a string, just use the `toString` method.

Since `tempResult` is just a product of two numbers, why not name it `product`. Also, you can use `const` here again.

For the most part, the other comments are good as they explain “why” you did something vs. explain what the code is doing.

1 Like

I agree,such comment of mine looks like not necessary explanation. I’ve just seen code examples where every function have descriptions like that no matter what’s in actual function body. I thought it was considered best practise to have expected input and expected output described for every function.

I definitely saw this type of commenting at the academic level but not sure how wide scale it is at large companies.

So for now I will stick to this:

If I have

``````veryDescriptiveFunctionName()
``````

If I have

``````vagueRidiculousVerboseLongFunctionName
``````

I need:
try to rename it first, maybe it will be better

if it not helps:
maybe function itself too complex >>> try to decompose it, split it

I think you have the right idea. Just remember comments should normally only explain “why” you do something and not “what” the code is doing. Many times I will comment line of code to explain why I might have chose a particular starting value if I am doing something non-traditional. However, some might argue to rewrite the code to be more obvious about what is going on and avoid the non-traditional code.

I’m on the other end of the spectrum with comments, but that’s because I write libraries.

In that context, writing docstrings for each function is important because that becomes the automatically generated documentation.

I think it’s generally considered a best practice to use === instead of == unless you explicitly intend to use type coercion.

I think that you can avoid a small amount od unneeded computation by starting with larger numbers and breaking the inner after a palendrome is found in the inner loop.

1 Like

You mean if I am looking for the largest, I can switch end steps and start steps and use decrement as an iteration step?
I tried that, but on earlier stages of implementation. I tried it in earlier versions of code, it didn’t work. But earlier code was crappy in general. I will try it.

You are referring to this line?

``````// if i == 2 >>> start step i = 10; end step i < 100

``````

If so, I think I get it.

The logic for your two loops is connected, right? You would need to change both.

Yeah, that’s the case

Yep, now it’s worked. New version(loops only).

``````    for (let i = (Math.pow(10, n) - 1); i > Math.pow(10, n - 1); i--) {
// if i === 2 >>> start step i = 99; end step i > 10

for (let j = i; j > Math.pow(10, n - 1); j--) {
// j = i >>> exploiting math pattern to reduce number of checks
// TODO: try to estimate big O notation for these nested loops for the sake of learning

let tempResult = i * j; /*TODO:
figure out what's best: use variable or end up with more multiplications*/

if (tempResult > result && isNumberPalindromic(tempResult)) {
result = tempResult;
break;
}
}
}
``````

I believe earlier I messed up something with Math.Pow, inside parenthesis.

You use this expression a few times. I think some (most?) Javascript engines recompute this value on every loop iteration. Something like `const smallestNDigitNumber = Math.pow(10, n - 1)` could add a bit of explanation and prevent the recalculation. Its a minor cost, but my rule of thumb is to store any calculation that is used multiple times (and not store a value only used once);

Got it. Better declare variable once than do bunch of calculations.