Extra Challange in WorkAround Explorer

Hello there, I want to ask if my code is written good (it works without any troubles)and if not please give me some advice on how to write better! :slightly_smiling_face: . I hope admins have all of the other code already saved and I can just put my “utitilies.js” here. So, here it is:

const formatWithComma = (intPart, i) => {
  let intPartAfter;
  if (i === 3) {
    intPartAfter = intPart.join('');
  } else {
    while (i > 3) {
      i -= 3;
      intPart.splice(i, 0, ',');
      intPartAfter = intPart.join('');
    }
  }
  return intPartAfter;
};
const checkIsNumber = (typeOfData) => {
  if (typeof typeOfData === 'number') {
    typeOfData = typeOfData.toString();
    return typeOfData;
  } else if (typeof typeOfData === 'string') {
    return typeOfData;
  } else return 'This is not a string or number';
};

const formatNumber = (number) => {
  number = checkIsNumber(number);
  const arrNum = number.split('.');
  const intPart = arrNum[0].split('');
  let i = intPart.length;
  number = formatWithComma(intPart, i);
  arrNum[0] = number;
  return arrNum.join('.');
};
export default formatNumber;

Hey @andriiantoniuk275266 I just finished this project and made post with my learnings, including my solution to the extra challenge if you want to take a look here.

There is no single way of coding a problem so getting a solution that works is already a major accomplishment. I like how you broke it down into smaller functions. Also how you are validating the input received is a number. (something the course doesn’t promote as much as it should) Since you asked, here are some of my humble suggestions on how your code can be refactored further or improved to make it more readable to others (and your future self) and ensure a more consistent result:

  1. checkIsNumber()

I would rename the checkIsNumber function to something like getStringOfNumber(). The current name reads as if a true or false type of value will return if the input is indeed a number. My suggestion is more in line with what the function is really doing. An alternative is separating the logic to check if you got a number from the logic of converting it into a string. (more on this in my point #3)
Second I would rename the input parameter from typeOfData to something like inputNumber or numberToCheck. The current name given is getting confusing with the action you are doing in your if statements (using the typeof command). It reads as if you are returning the type and not the actual number.

Third, In your number check validation you correctly look to see if the input is number. This would be the expected input for this type of function. Still if you look at my post linked, the course creators made this confusing as their Averaging functions are returning a string representation of a number. I see this as bad design on their part as it breaks the context of how things are named. Is also forcing you do the check if the input is type string then continue. Their averages should return a number.

Rant aside; to support a string parameter better, I suggest you validate that it is really possible to convert it to a number. In your current code, any string of text would pass your test and move on to the next section and get commas added. There are multiple ways to do this, but I chose to use the global is not a number (isNaN) check function as it already attempts to make the input string a number and works for both types of input. Doing this part brings your function in line with its original goal.

  1. formatWithComma()

First I would simplify by removing the i input parameter. This is just the length of the other parameter, which you can calculate inside your formatWithComma function instead of from calling function.
I would also move the logic of breaking the integer into its individual digits inside this function. You have that logic outside in the calling function, yet you are returning an already joined integer. Would be cleaner if you pass a full integer here and return a full integer too.

Second in your if (i === 3) you are missing the case where the integer part has 2 or 1 digits. Your function would return an undefined value as intPartAfter would not initialize; I would change this statement to if( i <= 3)

Third, in your while function for the case where there are more than 3 digits, you are inserting the comma every 3 digits, but then you are already joining your digits from inside the while loop into your return variable. If you have a number that needs multiple commas (4,000,000) you repeat this join in every iteration and overwrite the previous value. I would just do that join once after the while loop finishes inserting the commas.

  1. formatNumber(number)

Here the main problem I see is if your parameter received is not a number (the check returns “This is not a string or number”) you are still moving forward with the next steps. You would be adding commas to your error text. Ideally you would throw an exception error, from your check function and stop execution if you didn’t get a number. Alternatively you can return your error text to the caller of formatNumber() without moving into the next steps. This is where separating the logic of checking if you got a number and then converting that into a string would help. You could first check if you got a number and get a true or false answer. If not a number, return the error, otherwise continue with the split.

Second, you are modifying the input parameter ‘number’ multiple times within the function, using it as a temporary variable to hold different values during the execution. This can be confusing to other readers. I would avoid that and use the other temporary variables you already created inside the function. For example in this step:

  number = formatWithComma(intPart, i);
  arrNum[0] = number;

You can just save the return value directly into the arrNum[0] location.

With my other suggestions this entry part of your function would look more like this:

const formatNumber = (number) => {
    if(!checkIsNumber(number)) //Check just returns true or false
        return 'This is not a string or number';
    
    //split integer and decimal parts
    const arrNum = number.toString().split('.');

    //format integer part with commas
    arrNum[0] = formatWithComma(arrNum[0]);

    return arrNum.join('.');
  };

Sorry for the long post but I really wanted to provide you meaningful feedback. With coding, the first iteration of solving our problem renders code that has context spread in different areas as our thought process evolves. Further refactoring is needed to clean up the code, organize our final solution better, and make it more readable and easier to test and modify in the future.
This was a hard exercise when compared to the other projects in the learn JS course, so kudos for getting a working solution!