0
\$\begingroup\$

I have the following function which takes in an input and validates if that input is an Armstrong number or not:

An Armstrong Number is a number such that the sum of the cubes of its digits is equal to the number itself. For example, 371 is an Armstrong number since 3 ** 3 + 7 ** 3 + 1 ** 3 = 371.

Here's a link to the program on codepen.

const isArmstrong = input => {
  const expandedInputExpression = `
    ${input[0]} ** 3 + ${input[1]} ** 3 +
    ${input[2]} ** 3`;

  const expandedInputValue = Array.from(input)
    .map(digit => digit)
    .reduce((total, currentDigit) => total + currentDigit ** 3, 0);


  if (expandedInputValue === parseInt(input)) {
    console.log(`${input} is an Armstrong number since ${expandedInputExpression} = ${input}`);
    return true;
  } else {
    console.log(`
    ${input} is not an Armstrong number because ${expandedInputExpression}( ${expandedInputValue}) is not equal to ${input}`);
    return false;
  }
};

Is there any way to refactor and simplify the function?

\$\endgroup\$
  • \$\begingroup\$ outputDiv and outputText are not defined here so this function has an unstated side effect. Perhaps they should be passed in as an argument. Also, you've duplicated outputDiv.classList.remove("closed"); in both arms of an if/else. Can just put it once before or after the if/else. \$\endgroup\$ – jfriend00 Aug 12 '18 at 16:23
  • \$\begingroup\$ Also, the name of the function isArmstrong() implies it will return a value that tells you whether it is or isn't, but this doesn't do that. \$\endgroup\$ – jfriend00 Aug 12 '18 at 16:26
  • \$\begingroup\$ @jfriend00 Edited to remove unstated side effects and context \$\endgroup\$ – Oguntoye Aug 12 '18 at 17:03
0
\$\begingroup\$
  1. Being called isArmstrong, it should just return a boolean value, and not have side effects of setting any sort of DOM. That can be done in a separate function by the caller.

  2. expandedInputValue can probably have a better name. Maybe digitTotal or digitCubeTotal. But the current name isn't helpful.

  3. I think this is a no-op: .map(digit => digit)

\$\endgroup\$
  • \$\begingroup\$ You're right on all three points i have refactored the code >here< to reflect your suggestions \$\endgroup\$ – Oguntoye Aug 13 '18 at 5:52
0
\$\begingroup\$

As pointed out in earlier comments, the name isArmstrong() implies it will return a boolean value and so other concerns should not be handled by this function. instead it should be called as an helper in a parent function.

I have refactored and simplified the function as show below:

// input logic 
  const processInput = inputString => {

    // isArmstrong() should return a boolean 
    const isArmstrong = stringInt => {
      const digitCubeTotal = Array.from(stringInt)
        .reduce((total, currentDigit) => total + currentDigit ** 3, 0);
      return (digitCubeTotal === +stringInt) ? true : false;
    }

    // output presentation should be delegated to another function
    output(inputString, isArmstrong(inputString));

  };


  // output presentation
  const output = (input, isArmstrong) => {

    const expandedInputExpression = `
      ${input[0]} ** 3 + ${input[1]} ** 3 +
      ${input[2]} ** 3`; 

    if (isArmstrong) {
       console.log(`${input} is an Armstrong number since ${expandedInputExpression} = ${input}`);
    }  else {
       console.log(`
       ${input} is not an Armstrong number because ${expandedInputExpression} is not equal to ${input}`);
    }
  }
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.