trjhtr

I made a page to generate a random color with JavaScript and set that color to the background of the page.

//Global Variables
const hexChars = '0123456789ABCDEF';
const container = document.getElementById('container');
const hexText = document.getElementById('hex-text');

//Function to generate random hex code

function randomColor() 
	let hexCode = '#';
	for(i=0; i<=5; i++) 
 	let randomNum = Math.floor(Math.random() * 16);
 hexCode += hexChars[randomNum];
 
 return hexCode;


//Function to change page color & text

function applyColor() 
 let hexCode = randomColor();
	container.style.backgroundColor = hexCode;
 hexText.innerHTML = hexCode;


//Make random color on page load, click, and spacebar press.
window.onload = applyColor();
container.onclick = () => applyColor();
document.body.onkeyup = e => 
 if(e.keyCode == 32) 
 applyColor();
 

* 
 margin: 0;
 padding: 0;
 user-select: none;


#container 
 position: absolute;
 height: 100%;
 width: 100%;
 display: flex;
 align-items: center;
 justify-content: center;
 cursor: pointer;
 transition: background-color .4s;


#hex-text 
 font-family: sans-serif;
 font-size: 2em;
 color: white;
 background-color: rgba(150, 150, 150, .6);
 padding: .4em;
 border-radius: 6px;

<div id="container">
 <p id="hex-text">
 #000000
 </p>
</div>

How can I improve the efficiency or functionality of the JavaScript?

Edit: I'd also like to know if this is a fair generator, or if it's more likely to get one shade than another. I don't see why it would, but you never know.

Looks very good. And, as far as I know, your colour generator is fair.

For efficiency

you don't need to generate a random number for every hex character. You are choosing among 256^3 (16 777 216) colours, labeled with every number from 0x0 (0 in decimal) to 0xFFFFFF (256^3-1 in decimal, or 16 777 215).

So you can:

1. generate a random number let randomNum = Math.floor(Math.random() * 16777216) // 256^3
   


2. 
   

   use an inherent function for JavaScript Numbers, toString, to convert it to a string with an arbitrary radix:
   let hextest = randomNum.toString(16)

   
   
   

   By design, randomNum is a natural number (non-negative or 0) so you don't need code for validation).

   
   


3. 
   

   pad it with zeros. This question was asked a million times in Stack Overflow (this is only an example).

   
   
   

   For your example, where you don't need validation, hextest = ('00000' + hextest).substr(-6) will do. (After padding with all the zeros you'd ever need, picks only the last 6 characters.)

   
   


4. convert to uppercase and decorate: hextest = '#' + hextest.toUppercase(). Reference for toUpperCase()
   

This way you can get rid of the hexChars constant.

For functionality

I don't have any good idea on how to add functions. But I can suggest an improvement:

in the code you have to capture spacebar presses, add the line e.preventDefault(). This will prevent the default behaviour for the space key (which is scrolling a screen down).

That way, you can add all the content you want to your page without fear of accidentally scrolling down (and it won't scroll down Code Review when I try your embedded snippet, either).

Indentation

If you look at the live snippet in your question you can see the indentation is not right for some places, but if you open the snippet editor it suddenly looks ok. That's because you didn't use a consistent indentation. In some places you indented with Tab and in others with space

Be consistent and always use the same indentation.

Event handling

When you are registering an event that doesn't take any parameters, like your click event for the container div:

container.onclick = () => applyColor();

You can simplify it and not create an Arrow Function that just calls applyColor and instead use the function directly:

container.onclick = applyColor;

However it's preferable for multiple reasons to register the events with addEventListener. This allows you to:

• Register multiple events


• Make the event name clearer, since they don't have the on prefixed to them


• Make your intention clearer

So this last container click event would become:

container.addEventListener("click", applyColor);

Keep the momentum and change the rest of them:

window.addEventListener("load", applyColor);
container.addEventListener("click", applyColor);
document.body.addEventListener("keyup", e => 
 if(e.keyCode == 32) 
 applyColor();
 
);

Color generation

As others have already pointed out, and quite well i might add, there are simpler ways of generating a randomized color. Most of these alternatives don't even need a variable to hold the available symbols:

const hexChars = '0123456789ABCDEF';

This improves on making the code cleaner. In the end simpler is always better. But don't get me wrong, there is nothing wrong with the way you are doing it.

Key codes

In general you want to avoid memorizing things, or looking up things to be able to understand the code. In this case when i see the key code if:

if(e.keyCode == 32) 
 applyColor();

If i haven't memorized the entire ASCII table i may not know which key corresponds to 32, thus it will force me look it up (i actually did double check, because i wasn't 100% sure). Or i may think i know and actually not remember correctly and assume it is a different one.

This gets quickly out of hand if you use multiple keys:

if(e.keyCode == 32 || e.keyCode == 27 || e.keyCode == 9) 
 applyColor();

You can make the code more readable by creating constants for the keys you are using:

const SPACE_KEY = 32;

//rest of the code

if(e.keyCode == SPACE_KEY) 
 applyColor();

Other than what i already stated i can only see a minor tweak that you may want to do in the applyColor function which is a double assignment:

function applyColor() 
 container.style.backgroundColor = hexText.innerHTML = randomColor();

It makes it a bit wider, but it is a lot more concise.

Regarding your last edit question: it is fair generator and all colors are equally likely to be generated.

Question response

I'd also like to know if this is a fair generator, or if it's more likely to get one shade than another. I don't see why it would, but you never know.

It seems fair to me... the random function returns "A floating-point, pseudo-random number"^[1][1] so while the sequence could repeat, it might be very unlikely - refer to this SO answer for more of an explanation of that ... also read more about it in answers to this SO question: Will Math.random repeat?

Feedback:

• good use of constants


• good use of arrow functions - e.g. onclick function, onkeyup function

Suggestions:

• use const instead of let for anything that doesn't change: randomNum, e.g. hexCode - refer to this answer for basis


• 
  

  shorten the arrow function in onkeyup to use short-circuiting instead of if statement:

  
  
  

  document.body.onkeyup = e => e.keyCode == 32 && applyColor();
  

  
  
  

  This tends to be faster, though it may seem less readable to some (see performance test here)

  
  

The other answers have some good points, so I'm not going to repeat any of that.

const container = document.getElementById('container');
const hexText = document.getElementById('hex-text');

You are looking for elements without making sure they are even loaded yet. This only works here because the stack snippet puts the JavaScript at the bottom of the body, which is not standard. Normally the JavaScript is included from the head, in which case these will be null.

for(i=0; i<=5; i++)

i is not declared, so it's in the global scope. I also prefer i < 6 instead of i <= 5, since it loops 6 times. This is also how you would loop over an array or a string.

hexText.innerHTML = hexCode;

You are not parsing any HTML, so there are no reason to use .innerHTML. When you are just inserting text use .innerText.

window.onload = applyColor();

You are not assigning a function to handle the event, you are calling the function, and then assigning what it returns, which is nothing. It works because, as I said before, everything is already loaded at this point.