8
\$\begingroup\$

I have recently started learning JavaScript (A great language) and decided to start a small project which consists of a website with two inputs, one for hexadecimal input, and one for RGB input. When the user enters a valid color in either of the two inputs, then the background color of the website changes and the other input is updated with the same color in its corresponding format. Here is my code (CSS removed, as it doesn't pertain to the logic):

(function() {
    "use strict";
    
    window.onload = function() {
        let initialColorChoices = [
            "#0c6",
            "#099",
            "#c0c",
            "#39f",
            "#fa0",
            "#affd1b",
            "#d53e78",
            "#e04857",
            "#0cca9a",
            "#e6e600"
        ];
        
        let randomColor = initialColorChoices[Math.floor(Math.random() * 10)];
        
        document.body.style.backgroundColor = randomColor;
        
        let hexInput = document.getElementById("hex");
        let rgbInput = document.getElementById("rgb");
        
        hexInput.value = randomColor;
        rgbInput.value = convertHexToRGB(randomColor);
        
        // I've heard of something called arrow functions, how could 
        // I use those here?
        hexInput.addEventListener("change", function() {
            if (isValidColor(this.value)) {
                rgbInput.value = convertHexToRGB(this.value);
                document.body.style.backgroundColor = this.value;
            }  
        });
        
        rgbInput.addEventListener("change", function() {
            if (isValidColor(this.value)) {
                hexInput.value = convertRGBToHex(this.value);
                document.body.style.backgroundColor = convertRGBToHex(this.value);
            } 
        });
    };
    
    function convertHexToRGB(hexString) {
        let rgbColorValues = [];
        
        if (!isValidColor(hexString)) {
            return "";
        }
        
        if (hexString.length == 4) {
            for (const character of hexString.slice(1)) { // Excluding the #
                rgbColorValues.push(parseInt(character + character, 16));
            }
        }
        else {
            for (let i = 1; i < 7; i += 2) { // Starting at 1 to exclude #
                rgbColorValues.push(parseInt(hexString[i] + hexString[i + 1], 16));
            }
        }
        
        return `rgb(${rgbColorValues[0]}, ${rgbColorValues[1]}, ${rgbColorValues[2]})`;
    }
    
    function convertRGBToHex(rgbString) {
        let hexString = "#";
        
        if (!isValidColor(rgbString))
        {
            return "";
        }
        
        let rgbColorValues = rgbString.replace(/(\s)|(rgb\()|(\))/g, '') // Isolate the RGB colors by removing spaces, rgb(, and )
            .split(",") // Convert to string array
            .map(Number); // Convert to number array
        
        for (const color of rgbColorValues) {
            let currentColorString = color.toString(16);
            
            if (currentColorString.length < 2) {
                hexString += "0";
            }
            
            hexString += currentColorString;
        }
        
        return hexString;
    }
    
    function isValidColor(colorString) {
        const hexRegex = /^(#[0-9A-F]{3})|(#[0-9A-F]{6})$/i;
        const rgbRegex = /^rgb\(\s*\d{1,3}\s*,\s*\d{1,3}\s*,\s*\d{1,3}\s*\)$/;
        
        if (hexRegex.test(colorString)) {
            return true;
        }
        else if (rgbRegex.test(colorString)) {
            let rgbColorValues = colorString.replace(/(\s)|(rgb\()|(\))/g, '') // Isolate the RGB colors by removing spaces, rgb(, and )
                .split(",") // Convert to string array
                .map(Number); // Convert to number array
                        
            for (const color of rgbColorValues) {
                if (color < 0 || color > 255) {
                    return false;
                }
            }
            
            return true;
        }
        return false;
    }
}());
<!DOCTYPE html>
<html>
    <head>
        <title>Color Converter</title>
        
        <link href="resources/css/index.css" rel="stylesheet"/>
    </head>
    <body>
        <div class="container">
            <input class="color-input" id="hex" type="text" placeholder="Hex"/>
            <h1 id="heading">Color Converter</h1>
            <input class="color-input" id="rgb" type="text" placeholder="RGB"/>
        </div>
        <script src="resources/js/index.js"></script>
    </body>
</html>

(Ignore all the extra CSS classes, they are there for styling)

Here are the specific questions I want answered in this Code Review:

  1. Does my code use good JS/ES6 programming practices and proper coding style? (I tried to follow the airbnb style guide for this)
  2. Is my code well-structured and well-written?
  3. Should I give the IIFE function in my JS a name, so that it can be tested using a framework like Mocha (Maybe if I wanted to extract these functions into a small plugin), or should this code be structured differently in order to make it more maintainable and testable? (I believe I currently cannot access these functions inside this anonymous function)
\$\endgroup\$
  • \$\begingroup\$ There is a bug. rgb(100,4,100) converts to invalid color #64464 hex values less than 16 are missing leading zero. \$\endgroup\$ – Blindman67 Mar 6 '18 at 0:51
  • \$\begingroup\$ @Blindman67 I didn't realize that! What is the policy for such problems here? Should I make a change to the code? (I am reluctant since that could be considered as you still doing a code review) \$\endgroup\$ – Arnav Borborah Mar 6 '18 at 0:57
  • 1
    \$\begingroup\$ Change the code to remove the bug. Generally the code for review should be bug free. \$\endgroup\$ – Blindman67 Mar 6 '18 at 1:54
  • \$\begingroup\$ @Blindman67 I have fixed it! \$\endgroup\$ – Arnav Borborah Mar 6 '18 at 11:43
2
+50
\$\begingroup\$

Your Questions

  1. Does my code use good JS/ES6 programming practices and proper coding style? (I tried to follow the airbnb style guide for this)

For the most part I would say Yes. It caches DOM references in variables, though those could be stored in constants:

let hexInput = document.getElementById("hex");
let rgbInput = document.getElementById("rgb");

Because those are never re-assigned, it is wise to default to using the const keyword until a good reason for re-assingment arises.

const hexInput = document.getElementById("hex");
const rgbInput = document.getElementById("rgb");
  1. Is my code well structured and well written?

For the most part I would say Yes. However I do notice that the convert functions create a local variable (e.g. let rgbColorValues = [];) and then if the input value isn't valid, an empty string is returned. One could argue that the valid input check should come before declaring the local variable, lest memory be wasted (though in a small SPA like this it will likely be negligible).

  1. Should I give the IIFE function in my JS a name, so that it can be tested using a framework like Mocha (Maybe if I wanted to extract these functions into a small plugin), or should this code be structured differently in order to make it more maintainable and testable? (I believe I currently cannot access these functions inside this anonymous function)

I don't think giving it a name would be useful, though if you wanted to expose the methods, then perhaps the revealing module pattern would be good to follow for that.

  1. [from comment] Would my code benefit from separation into different files (Or even modules)? Say if I separated the color functions and UI into different files, and then used the revealing module pattern on the color functions? (Maybe I could move them into a class. I think that way I would be able to easily unit test my code easily)

With the current code, it seems like separating the functions into separate files would be a lot of extra network loading. I can't think of much of a benefit for that but if you have one then perhaps it would be worth it.

Other feedback

Arrow functions

// I've heard of something called arrow functions, how could 
// I use those here?

The biggest change would be removing the keyword function, and adding an arrow (i.e. =>) between the (empty) list of arguments and the function block (since there are multiple statements). Then because an arrow function "does not have its own this"1, this now refers to the enclosing scope, so change this.value to hexInput.value (in the first callback function, and then rgbInput.value in the second event callback function).

hexInput.addEventListener("change", () => {
        if (isValidColor(hexInput.value)) {
            rgbInput.value = convertHexToRGB(hexInput.value);
            document.body.style.backgroundColor = hexInput.value;
        }  
    });

Then another area that could be simplified using arrow functions is the end of the function isValidColor - specifically this block:

for (const color of rgbColorValues) {
    if (color < 0 || color > 255) {
      return false;
    }
  }
  return true;

Using a functional approach, an array method like find() could be used to look for any value that is outside that 0-255 range. If any such value is returned, then return false, otherwise if undefined is returned, return true:

return rgbColorValues.find(function(color) {
    return color < 0 || color > 255;
  }) === undefined;

And that can be simplified using an arrow function - to one line:

return rgbColorValues.find(color => color < 0 || color > 255) === undefined;

window.onload vs event listener

For a small page like this, it is fine to assign a function to window.onload. However if there is a need to have multiple functions executed when the page is loaded, then use window.addEventListener('load'); (similar to the event callbacks registered for the change events on the inputs). Also, the DOMContentLoaded event might be more appropriate than load.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the review! I have a question, though. Would my code benefit from separation into different files (Or even modules)? Say if I separated the color functions and UI into different files, and then used the revealing module pattern on the color functions? (Maybe I could move them into a class. I think that way I would be able to easily unit test my code easily) \$\endgroup\$ – Arnav Borborah Mar 15 '18 at 16:07
  • \$\begingroup\$ I added your question to my answer (see list item #4), along with a response \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Mar 15 '18 at 16:27
  • \$\begingroup\$ Thanks for your effort! This is going to help me a lot when I go forward! \$\endgroup\$ – Arnav Borborah Mar 15 '18 at 17:01
3
\$\begingroup\$

The code body asks about arrow functions, so I set out to write the converters with that in mind. In short, they can cut off clutter especially if you can make use of the implicit return value.

Another aspect I wanted to highlight (although not explicitly in the question) was to leverage first-class functions to give descriptive names to operations.

const radix = (from, to) => x => parseInt(x, from).toString(to)
, toHex = radix(10, 16)
, fromHex = radix(16, 10)
, compose = (...fs) => fs.reduce((f, g) => (...as) => f(g(...as)))
, pad = x => ("00" + x).slice(-2)
, doubled = x => x + x
, limit = x => Math.min(255, x).toString()
, rgb2hex = rgb => (rgb.match(/\d+/g) || [])
    .map(compose(pad, toHex, limit))
    .join('')
, hex2rgb = (hex, isCompact) =>
    (hex.match(isCompact ? /[0-9a-f]/gi : /[0-9a-f]{2}/gi) || [])
        .map(isCompact ? compose(fromHex, doubled) : fromHex)
        .join(',')
<html>
<body>
    <form name="convert" onsubmit="return false">
        <label><span>r,g,b</span>
            <input name="rgb" oninput="hex.value = rgb2hex(this.value)">
        </label>

        <label><span>hex</span>
            <input name="hex" oninput="rgb.value = hex2rgb(this.value, this.value.length <= 3)">
        </label>
    </form>
</body>
</html>

\$\endgroup\$
  • \$\begingroup\$ Wow! That just shows how powerful arrow functions are (It took me a while to understand what was going on here)! But is using that many and nesting them good practice? To me, it seems like (hex, isCompact) => (hex.match(isCompact ? /[0-9a-f]/gi : /[0-9a-f]{2}/gi) || []) .map(isCompact ? compose(fromHex, doubled) : fromHex) .join(',') is better phrased as a function. \$\endgroup\$ – Arnav Borborah Mar 7 '18 at 13:33
  • \$\begingroup\$ @ArnavBorborah yes, I actually tried a few options regarding that function; one was to make a dedicated function for the compact- and byte-versions, and a third one which made the decision on choosing between the two. In the end I came back to the single function version as the differences are side-by-side and that felt nicer to me. But I agree, it could be improved upon, I'm just not sure how. Is it good practice? I guess it's always a judgement call, but code reusability most certainly is a good thing. \$\endgroup\$ – morbusg Mar 7 '18 at 14:55
  • \$\begingroup\$ would an object literal or ES6 class make sense here to store all those different functions? \$\endgroup\$ – Aaron Goldsmith Mar 10 '18 at 23:02
  • 1
    \$\begingroup\$ @AaronGoldsmith classes make for a nice way to organize code. If you're not going to make use of instances of them, then a module makes more sense. Either way, it is customary in JS to avoid globality of variables like the plague (partly because the browser environment is so chock-full of them). Here, if all the app is going to do is this, then I don't see any benefit of organizing the code. But if it's going to be a part of a larger whole, then sure, a module would make most sense to me. \$\endgroup\$ – morbusg Mar 11 '18 at 7:41
  • \$\begingroup\$ @AaronGoldsmith or rather, modules, as for example the compose function is rather general and in no way tied to these converter functions. \$\endgroup\$ – morbusg Mar 11 '18 at 7:45

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.