3
\$\begingroup\$

I've started learning JavaScript last week. Today I ran into a JS exercise available at this link codewars exercise and decided to give it a try. I've used very basic JS because I know only a small number of JS methods. Could you please review and rate my code?

challenge text :

Your task is to write a function named do_math that receives a single argument. This argument is a string that contains multiple whitespace delimited numbers. Each number has a single alphabet letter somewhere within it.  

Example : "24z6 1x23 y369 89a 900b"

As shown above, this alphabet letter can appear anywhere within the number. You have to extract the letters and sort the numbers according to their corresponding letters.

Example : "24z6 1x23 y369 89a 900b" will become 89 900 123 369 246 (ordered according to the alphabet letter)

Here comes the difficult part, now you have to do a series of computations on the numbers you have extracted.

  • The sequence of computations are + - * /. Basic math rules do NOT apply, you have to do each computation in exactly this order.
  • This has to work for any size of numbers sent in (after division, go back to addition, etc).
  • In the case of duplicate alphabet letters, you have to arrange them according to the number that appeared first in the input string.
  • Remember to also round the final answer to the nearest integer.

Examples :
"24z6 1x23 y369 89a 900b" = 89 + 900 - 123 * 369 / 246 = 1299
"24z6 1z23 y369 89z 900b" = 900 + 369 - 246 * 123 / 89 = 1414
"10a 90x 14b 78u 45a 7b 34y" = 10 + 45 - 14 * 7 / 78 + 90 - 34 = 60

my code :

var inputStr, inputsArray, numsArray, arrayStrNumber, isAdd, isSub, isMul, isDiv;


numsArray = [];
strArray = [];
arrayStrNumber = [];
inputStr = "10a 90x 14b 78u 45a 7b 34y";

isAdd = true;
isSub = true;
isMul = true;
isDiv = true;

// create array from strings
inputsArray = inputStr.split(" ");

// loop in array

for (var i = 0; i < inputsArray.length; i++) {
  var inputValue = inputsArray[i];

  // seperate numbers from alphabet
  for (x = 0; x < inputValue.length; x++) {

    //   if it's not a number add it to string's array
    if (isNaN(inputValue[x])) {
      strArray[i] = inputValue[x];
    } else {
      isNaN(numsArray[i])
        ? (numsArray[i] = "" + inputValue[x] + "")
        : (numsArray[i] += "" + inputValue[x] + "");
    }
  }

  //create an array with letter on start
  arrayStrNumber[i] = strArray[i] + numsArray[i];
}
// sort array based on first alphabet or first number after alphabet
arrayStrNumber.sort(function (a, b) {
  if (a[0] != b[0]) {
    if (a > b) {
      return 1;
    }
    if (b > a) {
      return -1;
    }
    return 0;
  } else {
    if (arrayStrNumber.indexOf(a) > arrayStrNumber.indexOf(b)) {
      return 1;
    }
    if (arrayStrNumber.indexOf(b) > arrayStrNumber.indexOf(a)) {
      return -1;
    }
    return 0;
  }
});

console.log(arrayStrNumber);
// Do math operation with order +-*/
var finalResult = 0;
for (var i = 0; i < arrayStrNumber.length; i++) {
  arrayStrNumber[i] = parseInt(arrayStrNumber[i].slice(1));
  if (i === 0) {
    finalResult += arrayStrNumber[i];
    continue;
  }
  if (isAdd) {
    finalResult += arrayStrNumber[i];
    isAdd = false;
    continue;
  }
  if (isSub) {
    finalResult -= arrayStrNumber[i];
    isSub = false;
    continue;
  }
  if (isMul) {
    finalResult *= arrayStrNumber[i];
    isMul = false;
    continue;
  }
  if (isDiv) {
    finalResult /= arrayStrNumber[i];
  }
  isAdd = true;
  isSub = true;
  isMul = true;
  isDiv = true;
  finalResult = Math.round(finalResult);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Please copy the contents of the programming challenge into the question, as well as providing the link, links can break. \$\endgroup\$ – pacmaninbw May 17 at 14:53
  • \$\begingroup\$ Challenge text added to the question. Thanks for the tip \$\endgroup\$ – user3756068 May 17 at 15:27
2
\$\begingroup\$

When writing maintainable code, I'd recommend to declare variables in narrow scopes, if possible. If you do something like:

var inputStr, inputsArray, numsArray, arrayStrNumber, isAdd, isSub, isMul, isDiv;

at the very beginning, then on every line throughout the rest of the program, you'll have to keep in mind the possible values for all of those variables. If the script is long, this can be a problem. Consider encapsulating related functionality into separate functions instead - for example, you could turn the input into an array in one function, sort it in another, and then finally iterate through it to produce the result in a third. That way, for example, inputStr will only be visible to the first function (as an argument), and neither of the first two functions will have isAdd etc visible to them. Separating functionality into smaller self-contained sections makes code a lot more readable.

Since it's 2020, it would be nice to write source code in modern syntax - at least in ES2015. Modern syntax generally makes code more concise, easier to read, and less buggy. (I'll be using modern syntax in suggestions below.)

Consider always using strict mode - you have some variables that you never declare (strArray and x), which means that they get implicitly assigned to the global object. This is an easy source of bugs, not to mention inelegant; strict mode will throw an error when this sort of thing happens, allowing you to fix it on sight.

When you have to iterate over an array or a string, it's nice to be able to immediately work with each item being iterated over immediately, rather than having to mess with indicies. Both strings and arrays have iterators (which iterate over either each character, or each array item), so you can do this concisely with for..of. For example:

for (let i = 0; i < inputsArray.length; i++) {
  const inputValue = inputsArray[i];

can be replaced with

for (const inputValue of inputsArray) {

You do

isNaN(numsArray[i]) ?
  (numsArray[i] = "" + inputValue[x] + "") :
  (numsArray[i] += "" + inputValue[x] + "");

The conditional operator should only be used when you need an expression which is identified conditionally, like console.log(cond ? '1' : '2'). If the expression that results from using the conditional operator isn't being used, like in your code, it would be more readable to remove it and use if/else instead. (It's fine for minifiers to do that, but minified code isn't intended to be read - source code that developers write should be as readable as possible). There's even a linting rule to help you automatically identify and fix this sort of thing.

So, you have a string which is composed of numbers, plus one letter somewhere inside it, and you need to separate them. If you want to do this concisely, you might consider using replace to replace the letter character with the empty string, and use a callback to assign the matched value (the letter) to an outer variable, like this:

function separateWord(word) {
  let char;
  const num = Number(
    word.replace(
      /[a-z]/i,
      match => {
        char = match;
        return ''; // replace this match with the empty string
      }
    )
  );
  return { char, num };
}
console.log(separateWord('123b456'));

On a broader scale, using the above function, I think the first section of the code which turns each word into a sorted array would be easier to manage if you used an object indexed by character instead, whose values are arrays of numbers. Eg, for the above 123b456, that would result in the following object:

{
  b: [123456]
}

Iterate over each word, pushing to an array on the object, creating it first if necessary. Then, the object properties can be arranged lexiographically by sorting its entries (Object.entries returns an array of entries, where an entry is an array containing the key and the value, so just compare the keys in the .sort). This makes things so much shorter and cleaner:

function getOrderedNumbers(numbersByChar) {
  return Object.entries(numbersByChar)
    .sort((a, b) => (a[0] > b[0]) - 0.5) // Order entries alphabetically
    .map(entry => entry[1]) // Take only the value of each entry
    .flat(); // Turn the array of arrays of numbers into a single array of numbers
}
function parseString(inputStr) {
  const numbersByChar = {};
  for (const word of inputStr.split(' ')) {
    const { char, num } = separateWord(word);
    if (!numbersByChar[char]) {
      numbersByChar[char] = [];
    }
    numbersByChar[char].push(num);
  }
  const numbersArr = getOrderedNumbers(numbersByChar);
  return doMath(numbersArr);
}

For the doMath function, keeping track of 4 separate booleans and performing 4 different if checks is a bit verbose for the task at hand. You might consider using an array of functions instead, and using the modulo operator to identify which function to call:

function doMath(numbersArr) {
  const fns = [
    (subtotal, num) => subtotal + num,
    (subtotal, num) => subtotal - num,
    (subtotal, num) => subtotal * num,
    (subtotal, num) => subtotal / num,
  ];
  // Remove first value so operation can start with + with first and second value:
  let subtotal = numbersArr.shift();
  numbersArr.forEach((num, i) => {
    subtotal = fns[i % 4](subtotal, num);
  });
  return subtotal;
}
console.log(doMath([1, 2, 4, 5])); // ((1 + 2) - 4) * 5

reduce would be more appropriate than forEach to transform the array of numbers into a single number, but if you're a beginner, you probably prefer the forEach version since it's more intuitive:

function doMath(numbersArr) {
  const fns = [
    (subtotal, num) => subtotal + num,
    (subtotal, num) => subtotal - num,
    (subtotal, num) => subtotal * num,
    (subtotal, num) => subtotal / num,
  ];
  // Remove first value so operation can start with + with first and second value:
  const initialValue = numbersArr.shift();
  return numbersArr.reduce((subtotal, num, i) => fns[i % 4](subtotal, num), initialValue);
}
console.log(doMath([1, 2, 4, 5])); // ((1 + 2) - 4) * 5

Put it all together:

'use strict';

console.log(doMath("10a 90x 14b 78u 45a 7b 34y"));

function separateWord(word) {
  let char;
  const num = Number(
    word.replace(
      /[a-z]/i,
      match => {
        char = match;
        return ''; // replace this match with the empty string
      }
    )
  );
  return { char, num };
}
function getOrderedNumbers(numbersByChar) {
  return Object.entries(numbersByChar)
    .sort((a, b) => (a[0] > b[0]) - 0.5) // Order entries alphabetically
    .map(entry => entry[1]) // Take only the value of each entry
    .flat(); // Turn the array of arrays of numbers into a single array of numbers
}
// Entry point:
function doMath(inputStr) {
  const numbersByChar = {};
  for (const word of inputStr.split(' ')) {
    const { char, num } = separateWord(word);
    if (!numbersByChar[char]) {
      numbersByChar[char] = [];
    }
    numbersByChar[char].push(num);
  }
  const numbersArr = getOrderedNumbers(numbersByChar);
  return getTotal(numbersArr);
}

function getTotal(numbersArr) {
  const fns = [
    (subtotal, num) => subtotal + num,
    (subtotal, num) => subtotal - num,
    (subtotal, num) => subtotal * num,
    (subtotal, num) => subtotal / num,
  ];
  // Remove first value so operation can start with + with first and second value:
  let subtotal = numbersArr.shift();
  numbersArr.forEach((num, i) => {
    subtotal = fns[i % 4](subtotal, num);
  });
  return Math.round(subtotal);
}

Unfortunately, the latest JS version that Codewars supports is Node 8, which is old, end-of-life, and probably shouldn't be used - it doesn't support Array.prototype.flat. An alternative to achieve the same functionality is to spread into concat instead:

arr.flat()

can be replaced by

[].concat(...arr);
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Honestly I didn't expect to get review at all! You surprise me with this detailed answer. Thank you very much for taking time to write this awesome instruction. I don't know much about JS because this is my second week with JS and still learning :) and will try to do better next time. \$\endgroup\$ – user3756068 May 18 at 11:10

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.