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.

Challenge link

Discussions related to implementation:
About code structure
Optimization

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

Would appreciate feedback not only about algorithm, but about comments in code.

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()

there is no need for additional comments.

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

if nothing of it possible >>> add more comments

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.