Randomly generate a Hex Colour code and its contrasting colour using JavaScript

Question

I am attempting to generate a new colour and its contrasting colour with this javascript code.

I am making a web app that is different every time it is loaded (i.e. background and text colour)

I would love some critique on the quality and efficiency of my code.

How can it be done better?

Here is my code:

function startcolour() {
 var values = Array('1','2','3','4','5','6','7','8','9','A','B','C','D','E','F');
 var color = "#";
 var item;
 for (i = 0; i < 6; i++) {
 item = values[Math.floor(Math.random()*values.length)];
 color = color + item;
 }
 return color;
}
function contrastcolour(colour) {
 var r;
 var g;
 var b;
 var comp = [];
 var contrast = '#';
 var arrayLength;
 if (colour.charAt(0) == '#') {
 colour = colour.slice(1);
 }
 r = (255 - parseInt(colour.substring(0,2), 16)).toString(16);
 g = (255 - parseInt(colour.substring(2,4), 16)).toString(16);
 b = (255 - parseInt(colour.substring(4,6), 16)).toString(16);
 contrast = contrast + padZero(r) + padZero(g) + padZero(b);
 return contrast
}
function padZero(str) {
 if (str.length < 2) {
 return '0' + str;
 }
 else {
 return str;
 }
}
function colourRun() {
 var colours = [];
 var base = startcolour();
 colours.push(base);
 var contrast = contrastcolour(base);
 colours.push(contrast);
 return colours;
}

Sample output (aka colours):

['#123abc','#edc543']

I then do the following with jQuery to set CSS properties of HTML components:

 $(document).ready(function() {
 var colours = colourRun();
 $("body").css({"background-color": colours[0], "color": colours[1]});
 $("nav").css({"background-color": colours[1], "color": colours[0]});
 });

All constructive criticism is appreciated.

Answer 1

There is a similar question that you may want to take a look at for some extra insight, even thought it doesn't have the contrasting color feature:

JavaScript Random Color Generator

Now lets get to the review.

Naming

• The common naming convection in JS for methods is camelCase which you didn't follow in general.

• Most of the variable and function names aren't clear or descriptive of what they do/hold. This is a lot more important than it may seem at first, and not naming properly makes your code difficult to read and understand.

  As a side note Uncle Bob has a full chapter on this topic on his Clean Code book.

• Be consistent in the names. If you look closely you'll see that in some places you used color:

  var color = "#";
  

  While in others colour:

  function contrastcolour(colour) {
  

  This is at best confusing if not mysterious. Leaves one wondering if it is a different thing or if it has a different meaning.

  For this review i went with colour which was the one you used more often.

Functions

Now an overall review of the functions you presented:

• startcolour

  Better than using an array of elements is using a string with all possible characters:

  var values = '123456789ABCDEF';
  

  You can still use the index operator to fetch a character as you do in a normal array.

  There is no need to separate the '#' in a single variable and then add only the color part to a different one, and the += operator is also more idiomatic and still easy to read.

  Don't use variables without declaring them, as you have in the for:

  for (i = 0; i < 6; i++) {
  //---^
  

  i wasn't declared at all which makes it global, and may be the source of some hard to find bugs.

  Taking all this into consideration the function could be rewritten to:

  function randomColour(){
   const chars = '123456789ABCDEF';
   let colour = '#';
   for (let i = 0; i < 6; i++) {
   const randomIndex = Math.floor(Math.random() * chars.length);
   colour += chars[randomIndex];
   }
   return colour;
  }
  

  Or you can take a totally different approach, the one mentioned in the related question:

  function randomColour(){
   const randNum = Math.floor(Math.random() * 16777216); //from 0 to ffffff
   return '#' + randNum.toString(16).padStart(6,"0");
  }
  

  This generates a number between 0 and 16777215, or ffffff in hexa, and outputs it with hexadecimal formatting, by calling toString(16). It is then padded with zeros to a length of 6.

• padZero

  There is no need to create a padding function, because there is one already, called padStart or padEnd. It even has a polyfill if you need to support some old browsers.

  Using it in your code would look like so:

  r.toString().padStart(2,"0")
  

  Note that it's a method on string therefore if i have a number i must first transform it to string.

• colourRun

  The name on this one is also not very obvious to what it does. If the function returns a random color and it's contrast a better name would be baseContrastColours.

  Its important to mention that push supports any number of elements, so there is no need to make two separate pushes. Also in this specific case its easier to just return the array literal with the respective elements instead of creating the array, pushing the elements and returning at the end.

  Like this:

  function baseContrastColours() {
   const base = randomColour();
   const contrast = contrastColour(base);
   return [base, contrast];
  }
  

  While it has the same functionality, its simpler and easier to read.

• contrastcolour

  All the r, g, b variables are created up top and only set below:

  function contrastcolour(colour) {
   var r;
   var g;
   var b;
   ...
   r = (255 - parseInt(colour.substring(0,2), 16)).toString(16);
   g = (255 - parseInt(colour.substring(2,4), 16)).toString(16);
   b = (255 - parseInt(colour.substring(4,6), 16)).toString(16);
   ...
  }
  

  You want to create variables and set the values directly if there is nothing else to do in between both actions:

  var r = (255 - parseInt(colour.substring(0,2), 16)).toString(16);
  

  Considering the variables were declared with var they are even hoisted. Also in general you want to declare the variables as close as possible to were you use them.

  The arrayLength and comp variables aren't used. Always remove unused variables to keep the code as clean as possible.

  Also important is that you are repeating the inverse logic 3 times, one for each color channel. So while simple you may want to consider abstracting that logic to a separate function.

  So this function could be rewritten as follows:

  function inverseChannelColour(channelColour){
   return (255 - parseInt(channelColour, 16)).toString(16);
  }
  function contrastColour(colour) {
   if (colour.charAt(0) == '#') {
   colour = colour.slice(1);
   }
   const r = inverseChannelColour(colour.substring(0,2));
   const g = inverseChannelColour(colour.substring(2,4));
   const b = inverseChannelColour(colour.substring(4,6));
   const contrast = '#' + r.toString().padStart(2,"0") 
   + g.toString().padStart(2,"0") 
   + b.toString().padStart(2,"0");
   return contrast;
  }
  

As pointed out by @Zeta in the comments if the color to be inverted is a gray very close to the middle, the inversion will generate a similar color which may not be easy to read.

You can try to avoid this by manually checking the difference of all channels to their inverted versions, and if all of them are bellow a certain threshold generate a different color. A simple approach to this would be to generate black if all channels are closer to white and white otherwise.

There are also some libraries/micro-libraries that generate colors and their complementary versions with other approaches, such as using HSL. These may be interesting for you to take a look.

Here are some of them for reference:

• invert-color
• Colors.js
• complementary-colors

• Zeta: I'd write 0x1000000 instead of Math.pow(256,3), to be honest. By the way, the inverseChannelColour will return 127 on 128, but 0x7f7f7f is not that different from 0x808080. Feel free to add that to your review.
• Isac: @Zeta Thank you for the comment. I went with the Math.pow(256, 3) to make it more readable but i do agree that its more efficient to have the calculated number.
• Fendec: This is awesome! I really appreciate all the help. I am trying to enhance and correct my coding as I go.
• Blindman67: Would not 2 << 23 or 2 ** 24 give a more readable / explanative representation of the needed number?
• Isac: @Blindman67 I was torn between multiple options, but i think declaring a const up top may be the best one for readability. And 1 << 24 seems quite reasonable as well