3
\$\begingroup\$

As a beginner coder in js, I'd like to hear about further improvements of the following code, but nothing too advanced, please.

The program is meant to work the following way:

  1. A user inputs a color array like this one: ["blue", "green"]
  2. The code checks if those words are valid inputs (they should be found in a reference array of COLORS). This is done with catchInvalid function
  3. If they are valid inputs, and so they are found in COLORS array, it gets the index of it. This is done in the decodedValue function

Example1:

COLORS= ["blue", "yellow", "red"]
User input: userColors=["yellow"]
returns index of yellow in COLORS i.e 1. 

Example2:

COLORS= ["blue", "yellow", "red"]
User input: userColors=["yellow", "red"]
returns index 12 (index of yellow in colors*10 + index of red in colors (2)).

That's all. It's now working fine, but I wonder if you'd give any suggestions for improving the code.

    const COLORS = ["black", "brown", "red", "orange", "yellow", "green", "blue", "violet", "grey", "white"];
 //this will be the reference array
    const catchInvalid = (color, COLORS) =>{
    //checks if color is in COLORS (which is COLORS)
            if(COLORS.indexOf(color)==-1){
                    return `not a ${color} in ${COLORS}`
            }
            else { }
    }
    
    const decodedValue = (colorArray) => {
            //if previous 'color' is in the reference array, get the index of the color in COLORS.
    let CODES=[];
    
    if (colorArray.length==0){
            return "Input a color value"
            }
    else if (colorArray.length==1){
            catchInvalid(colorArray[0], COLORS)
            return COLORS.indexOf(colorArray[0])
            }
    else {
    for(let i=0; i<2;i++){
            //only for the first 2 items in the array.
            catchInvalid(colorArray[i], COLORS)
            CODES.push(COLORS.indexOf(colorArray[i]))
            }
                                                                                                            return CODES[0]*10 + CODES[1];
        }
}

console.log(decodedValue(["blue"]), decodedValue(["nothing"]), decodedValue(["blue", "green"]))
\$\endgroup\$
2
  • \$\begingroup\$ Welcome to CodeReview \$\endgroup\$
    – konijn
    Jun 22, 2020 at 13:46
  • \$\begingroup\$ thx again :-) @konijn \$\endgroup\$ Jun 22, 2020 at 22:05

1 Answer 1

4
\$\begingroup\$

From a short review, considering you are a beginner;

  • keep your variables in lowerCamelCase so
    • COLORS -> colors
    • CODES -> codes
  • on the whole, avoid to have data type in the variable name
    • colorArray -> colors
  • your commenting is quite good
  • Your indenting is inconsistent, it easier to read your code when code is properly indented
  • catchInvalid probably should return a boolean
  • I would have called catchInvalid -> isInvalidColor it gives more detail
  • catchInvalid should either use the global, or know the colors locally
  • I would use COLORS.includes() over COLORS.indexOf
  • There is way to calculate the return code without making a distinction between 1 or 2 elements

Per the comment, a bit more explicit

    function decodeColorValues(colors){
      //These are all the possible colors
      const knownColors = ["black", "brown", "red", "orange", "yellow", "green", "blue", "violet", "grey", "white"];        

      //Functions should return a consistent datatypes, so I return -1 instead of a message
      //If the caller did not provide an aray but say "orange", then this will return -1 as well
      if(!colors.length){
        return -1;
      }
      
      //Filter out unknown colors
      colors = colors.filter(color => knownColors.includes(color));

      //If all colors were unknown then return -1
      //You could change this so that if 1 color is unknown it returns -1
      if(!colors.length){
        return -1;
      }      
      
      //We only deal with the first 2 entries (why?)
      colors = colors.slice(0,2);
      let value=0;
      
      //Abuse the fact that 10 times zero is still zero
      for(const color of colors){
        value = value * 10 + knownColors.indexOf(color);
      }
       
      return value;
    }


    console.log(decodeColorValues(["blue"]),
                decodeColorValues(["nothing"]),
                decodeColorValues(["blue", "green"]),
                decodeColorValues(["blue", "nothing"])
           );
\$\endgroup\$
5
  • \$\begingroup\$ I agree with everything you've stated here, but I don't agree with stating to a newcomer (or anyone, ever, in fact) that something should be done this way, simply because I said so. This doesn't help the person understand what makes one way of doing something better than another, and learning effectively only comes through understanding. I'll pre-empt a possible response that some would cite "good coding practice" as the reason, which is not a reason, but a name given to a collection of guidelines that often don't come with an explanation. \$\endgroup\$
    – CJK
    Jun 22, 2020 at 16:20
  • \$\begingroup\$ @CJK You must be new to CodeReview, welcome! \$\endgroup\$
    – konijn
    Jun 22, 2020 at 19:32
  • \$\begingroup\$ thanks both :-) @CJK \$\endgroup\$ Jun 22, 2020 at 22:03
  • \$\begingroup\$ Would you be more explicit in the last bullet point, please? \$\endgroup\$ Jun 22, 2020 at 22:57
  • \$\begingroup\$ @misternobody Done! \$\endgroup\$
    – konijn
    Jun 24, 2020 at 8:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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