I have submitted the following bit of script as the answer to one of the tasks I was set by a recruiter. My submission has already been made and cannot be altered. However, I am keen to ask how you would improve the solution.

Task:

Using Javascript and no library or odd ons, create a script that can take 2 HTML Hex colour strings and return the average (mean) colour, it must show use of ES5 or ES6 (EcmaScript 5 or 6)

var arrColours = new funColours();
function funColours (){
	this.colours = [{
			c1:{r:0, g:0, b:0},
			c2:{r:0, g:0, b:0}, 
			c3:{r:0, g:0, b:0}
	}];
}

function funUpdateTxtColour(intFormElNo){
	let elHexColour = document.querySelector("#elFrmColourSelector" + intFormElNo);
	let elHexTxt = document.querySelector("#elHexCol" + intFormElNo);
	if (typeof(elHexColour) == "undefined" || typeof(elHexTxt) == "undefined" ){
		funErrorHandler("Element not detected, please check HTML or JavaScript calls to funUpdateColour", "elHexCol" + intFormElNo + " and elFrmColourSelector" + intFormElNo);
		return;
	}
	elHexTxt.value = elHexColour.value;
	let strOutputNode = ("Updated colour: " + intFormElNo + " with the value " + elHexTxt.value + " and applied to both form elements");
	funCalculateMeanColour("elAvgColourTxt");
}
function funUpdateColourSelector(intFormElNo){
	let strElHexColour = document.querySelector("#elHexCol" + intFormElNo);
	let elColourSelector = document.querySelector("#elFrmColourSelector" + intFormElNo);
	if (strElHexColour.length != 7 ){
		alert("HTML Hex colour codes are 7 characters in length and start with a # symbol");
		let intHashCheck = strElHexColour.value.indexOf("#");
		if (intHashCheck != 0 )	{
			if (strElHexColour.value.length == 6){
				funErrorHandler("User inputted a colour code without the # symbol, prefixing entry.", "funUpdateColourSelector");
				strElHexColour.value = "#" + strElHexColour.value;
			}else{
				funErrorHandler("User inputted a colour code without the # symbol.", "funUpdateColourSelector");
				return;
			}
		}
		strElHexColour.focus();
	}
	elColourSelector.value = strElHexColour.value;
	funCalculateMeanColour();
	return;
}
function funCalculateMeanColour(elMeanColour){
	let strHexColour1 = document.querySelector("#elHexCol1").value;
	let strHexColour2 = document.querySelector("#elHexCol2").value;
	let elColourBox = document.querySelector("#elAvgColour").style;
	let elAvgColourLabel = document.querySelector("#elAvgColourTxt");
	arrColours.colours[0].c1.r = funHexToDec(strHexColour1, 1,3);	
	arrColours.colours[0].c1.g = funHexToDec(strHexColour1, 3,5);	
	arrColours.colours[0].c1.b = funHexToDec(strHexColour1, 5,7);	
	arrColours.colours[0].c2.r = funHexToDec(strHexColour2, 1,3);	
	arrColours.colours[0].c2.g = funHexToDec(strHexColour2, 3,5);	
	arrColours.colours[0].c2.b = funHexToDec(strHexColour2, 5,7);	
	arrColours.colours[0].c3.r = funReturnHEXAverage(arrColours.colours[0].c1.r, arrColours.colours[0].c2.r);
	arrColours.colours[0].c3.g = funReturnHEXAverage(arrColours.colours[0].c1.g, arrColours.colours[0].c2.g);
	arrColours.colours[0].c3.b = funReturnHEXAverage(arrColours.colours[0].c1.b, arrColours.colours[0].c2.b);
	let strHex = "#" + arrColours.colours[0].c3.r + arrColours.colours[0].c3.g + arrColours.colours[0].c3.b;
	elColourBox.background = strHex;
	elAvgColourLabel.innerHTML = strHex;
}

function funHexToDec(strHexCode, intStart, intEnd){
	return parseInt(strHexCode.substring(intStart,intEnd), 16);
}

function funReturnHEXAverage(intValue1, intValue2){
	intAvg = Math.ceil((intValue1 + intValue2) / 2);
	return intAvg.toString(16);
	
}

function funErrorHandler(errString, elName){
	console.log("Error:");
	console.log(errString);
	console.log("Element: " + elName);
}
/* Task 1 */
div#frmColorContainer{
	padding: 10px;
	border-style: inset;
	border-width: 1px;
	border-color: #C0C0C0;
	background: silver;
	border-radius: 7px;
	width: 300px;
	height: 84px;
	font-family: arial;
}
div#elAvgColour{
	position: absolute;
	left: 340px;
	top: 10px;
	border: 2px solid grey;
	background: #808080;
	height: 100px;
	width: 100px;
}
label#elAvgColourTxt{
	position: relative;
	top: 6px;
	
}
label#elAvgColourTxt::before{
	content: 'Average Colour: ';
}
<!HTML>
	<head>
		<title>Task 1: VueJS Role - Candidate: xxx xxxx</title>
		<link href="css/css.css" rel="stylesheet">
		<script src="js/task1.js"></script>
	</head>
	<body>
		<form id="frmEls" method="get">
			<div id="frmColorContainer">
				<label>Colours:</label>
				<label>HEX:</label><br>
				<label>Colour 1:</label><input type="color" id="elFrmColourSelector1" value="#000000" title = "Colour 1" onchange="funUpdateTxtColour(1)">&nbsp;<input type="text" id="elHexCol1" value="#000000" onchange="funUpdateColourSelector(1)" maxlength=7><br>
				<label>Colour 2:</label><input type="color" id="elFrmColourSelector2" value="#FFFFFF" title="Colour 2"  onchange="funUpdateTxtColour(2)">&nbsp;<input type="text" id="elHexCol2" value="#FFFFFF" onchange="funUpdateColourSelector(2)" maxlength=7>
				<label id="elAvgColourTxt">#808080</span></label>
			</div>
		</form>
		
		<div id="elAvgColour">
		</div>
	</body>
</html>

migrated from codereview.meta.stackexchange.com Sep 13 at 16:10

This question came from our discussion, support, and feature requests site for peer programmer code reviews.

up vote 5 down vote accepted

First of all I would define a function that takes two valid HTML color strings and returns a valid HTML color string that represents the average of the two.

With this approach you could avoid polluting your "color library code" with calls to host objects, which helps for better code reuse. DOM manipulation and input validation (e.g. by a regex) could then be done in seperate functions.

Your solution returns #0080 as the average of #0000ff and #000000. This is because you do not format the RGB values separately with leading zeros.

To fulfill the ECMAScript 5 requirement I would suggest strict mode.

Overall feedback

The code feels a bit overly-complex. For instance, it seems like a lot of overhead to make a function to get the colours object, and use that to create an object, when one could simply create an object. Also, having every function start with the letters fun feels foreign - presumably this is a familiar convention from other programming languages but it is rare to see in JavaScript code.

Flaws

Checking the length of an element (or null) instead of value

The code in funUpdateColourSelector() utilizes document.querySelector(), which either returns "the first Element within the document that matches the specified selector, or group of selectors " or " If no matches are found, null is returned."1. So the code below checks the length of the element:

let strElHexColour = document.querySelector("#elHexCol" + intFormElNo);
let elColourSelector = document.querySelector("#elFrmColourSelector" + intFormElNo);
if (strElHexColour.length != 7 ){

Perhaps you intended to check the value property of that element?

Extra closing span tag

There is a superfluous closing tag: </span>. Remove that to make the HTML more valid.

<label id="elAvgColourTxt">#808080</span></label>

Suggestions

Use const unless value needs to be re-assigned

Many variables are declared with let, given a value and never re-assigned. It is recommended to default to using const and when a need for re-assignment arises (e.g. iterating in a loop, etc.) then use let.

Cache DOM References in variables

Querying for DOM elements (e.g. using querySelector()) is not very cheap. It is wise to store elements that get used frequently in a variable (perhaps using const) once the DOM is ready.

Use document.getElementById() instead of document.querySelector()

Using document.querySelector() is like using a tank to hammer in a nail... well maybe not that extreme but along those lines. See this jsPerf for a comparison.

Don't mix JavaScript in with the HTML

This requires a bit of a rework (see code below) but it is wise to separate the layout/markup (HTML) from the logic (JavaScript). The functions called from the onchange attributes can be moved into a callback fired whenever a change is fired on the document:

document.body.addEventListener('change', event => {
    //use event.target as the input that was changed
});

Also notice in the code below that name attributes were added to the input elements and then those are used to find the sibling to update.

Use more arrow functions

Using arrow functions can greatly reduce the number of lines required. For example,

function funHexToDec(strHexCode, intStart, intEnd){
    return parseInt(strHexCode.substring(intStart,intEnd), 16);
}

Can be reduced to:

const funHexToDec = (strHexCode, intStart, intEnd) => parseInt(strHexCode.substring(intStart,intEnd), 16);

Rewrite

See the code below that is dramatically simplified.

const validateValue = value => {
  if (value.length != 7) {
    alert("HTML Hex colour codes are 7 characters in length and start with a # symbol");
    return false;
  }
    const intHashCheck = value.indexOf("#");
    if (intHashCheck != 0) {
      if (value.length == 6) {
        funErrorHandler("User inputted a colour code without the # symbol, prefixing entry.", "funUpdateColourSelector");
      } else {
        funErrorHandler("User inputted a colour code without the # symbol.", "funUpdateColourSelector");;
      }
      return false;
    }
    return true;
}

//pad values less than 10 with a leading 0
const padValue = value => value < 10 ? '0' + value: value;
//convert hex to decimal
const hexToDec = (strHexCode, intStart, intEnd) => parseInt(strHexCode.substring(intStart, intEnd), 16);
//calculate average of hex numbers
const HEXAverage = (intValue1, intValue2) => {
  intAvg = Math.ceil((intValue1 + intValue2) / 2);
  return intAvg.toString(16);
}

document.addEventListener('DOMContentLoaded', _ => {
  //cache DOM references
  const elColourBox = document.getElementById("elAvgColour");
  const elAvgColourLabel = document.getElementById("elAvgColourTxt");
  document.body.addEventListener('change', event => {
    if (!validateValue(event.target.value)) {
      event.target.focus();
      return;
    }
    let siblingName;
    if (event.target.name.indexOf('color') > -1) {
      siblingName = event.target.name.replace('color', 'text');
    } else {
      siblingName = event.target.name.replace('text', 'color');
    }
    const siblingInput = document.forms[0].elements[siblingName];
    siblingInput.value = event.target.value;
    funCalculateMeanColour();
  });

  function funCalculateMeanColour(elMeanColour) {
    const strHexColour1 = document.forms[0].elements.colorInput1.value;
    const strHexColour2 = document.forms[0].elements.colorInput2.value;
    const r1 = hexToDec(strHexColour1, 1, 3);
    const g1 = hexToDec(strHexColour1, 3, 5);
    const b1 = hexToDec(strHexColour1, 5, 7);
    const r2 = hexToDec(strHexColour2, 1, 3);
    const g2 = hexToDec(strHexColour2, 3, 5);
    const b2 = hexToDec(strHexColour2, 5, 7);
    const r3 = HEXAverage(r1, r2);
    const g3 = HEXAverage(g1, g2);
    const b3 = HEXAverage(b1, b2);
    const strHex = "#" + padValue(r3) + padValue(g3) + padValue(b3);
    elColourBox.style.background = strHex;
    elAvgColourLabel.innerHTML = strHex;
  }
});

function funErrorHandler(errString, elName) {
  console.log("Error:");
  console.log(errString);
  console.log("Element: " + elName);
}
/* Task 1 */

div#frmColorContainer {
  padding: 10px;
  border-style: inset;
  border-width: 1px;
  border-color: #C0C0C0;
  background: silver;
  border-radius: 7px;
  width: 300px;
  height: 84px;
  font-family: arial;
}

div#elAvgColour {
  position: absolute;
  left: 340px;
  top: 10px;
  border: 2px solid grey;
  background: #808080;
  height: 100px;
  width: 100px;
}

label#elAvgColourTxt {
  position: relative;
  top: 6px;
}

label#elAvgColourTxt::before {
  content: 'Average Colour: ';
}
<form id="frmEls" method="get">
  <div id="frmColorContainer">
    <label>Colours:</label>
    <label>HEX:</label><br>
    <label>Colour 1:</label><input type="color" id="elFrmColourSelector1" value="#000000" title="Colour 1" name="colorInput1">&nbsp;<input type="text" id="elHexCol1" value="#000000" maxlength=7 name="textInput1"><br>
    <label>Colour 2:</label><input type="color" id="elFrmColourSelector2" value="#FFFFFF" title="Colour 2" name="colorInput2">&nbsp;<input type="text" id="elHexCol2" value="#FFFFFF" maxlength=7 name="textInput2">
    <label id="elAvgColourTxt">#808080</label>
  </div>
</form>
<div id="elAvgColour">
</div>

1https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelector

If I saw this code as an interview submission, I'd see some significant issues:

  • Separation of concerns: Your average-of-two-colours calculations are intermingled with the UI code. There should be an avgColor(hexColor1, hexColor2) function that is purely responsible for the calculation, and that function should be well documented. The rest of the code, then, would be glue that makes the form work.

    The way I interpret the task, it is primarily concerned about calculating the mean of two colours. Your solution, though, is primarily focused on form fields and the DOM. It's possible that you may have missed the point of the exercise. In my opinion, the avgColors() function in my solution below is a sufficient answer; the rest of the code is there as packaging just to demonstrate that it works.

  • Pointless use of arrays and objects: Why do you use arrColours.colours[0].c1, arrColours.colours[0].c2, and arrColours.colours[0].c3? You could have just used three variables, c1, c2, and c3. This, to me, is a red flag that you have no idea what arrays and objects are for.
  • Hungarian and funny naming: What are funColours? In seriousness, I find the rampant use of Hungarian prefixes annoying. (I know, this is just my opinion, but first impressions do matter in interview questions.) The worst examples are funCalculateMeanColour and funReturnHEXAverage — shouldn't most functions calculate and return results?

    I'd also quibble about funHexToDec: the result is a number. You shouldn't consider it to be in "decimal" form. The number only becomes decimalized when you convert it to a string representation.

  • Namespace pollution: JavaScript allows functions to be defined within the scope of other functions. I would strongly recommend decluttering your code by not defining helper functions in the global scope.

    In my solution below, I've defined a global avgColor() function (because it's what the task asks for), an AvgColorCalculator class (containing all of the UI glue code for the demonstration), and one statement that instantiates the calculator.

Some more minor concerns:

  • It's unclear, from the problem specification, what the input formats may be. CSS, for example, accepts a shorthand form consisting of three hex digits, so it would be better to accept that as well.
  • It would be smarter to make the HTML elements for presenting the output resemble the elements that accept the input. I suggest using the same kinds of <input> elements, but disabled. I'm also puzzled why you put the "Average Colour:" label text in CSS rather than in HTML.
  • document.querySelector() is overkill, when you only need document.getElementById(). I suggest using data attributes so that correspondences between the pairs of <input> elements are encoded within the HTML form itself, rather than hard-coded in the JavaScript.

Suggested solution

/**
 * Average of two colors in RGB color space.  Inputs may be of the form
 * "#6699FF", "#69f", "6699ff", or "69F".  Output is of the form "#6699ff".
 */
const avgColor = (hexColor1, hexColor2) => {
    let int24 = (hex) => parseInt(
        hex.replace(/^#?(?:([0-9a-f]{6})|([0-9a-f])([0-9a-f])([0-9a-f])|.*)$/i,
                    '$1$2$2$3$3$4$4'),
        16
    );
    let c1 = int24(hexColor1);
    let c2 = int24(hexColor2);
    if (isNaN(c1) || isNaN(c2)) return null;

    let maskedMean = (mask) => (((c1 & mask) + (c2 & mask)) >> 1) & mask;
    let avg = maskedMean(0xff0000) | maskedMean(0x00ff00) | maskedMean(0x0000ff);
    return '#' + ('000000' + avg.toString(16)).slice(-6);
};

class AvgColorCalculator {
    constructor(colorSel1, colorSel2, colorOut) {
        this.colorText1 = this.colorInputField(colorSel1);
        this.colorText2 = this.colorInputField(colorSel2);
        this.colorOut = colorOut;
        this.textOut = document.getElementById(colorOut.getAttribute('data-mirror'));
        this.recalculate();
    }
    
    /**
     * Set up event handlers.  When the given color selector changes, mirror
     * its value to the corresponding text field, and vice versa, then update
     * the output fields.
     */
    colorInputField(colorSel) {
        let calc = this;
        let textField = document.getElementById(colorSel.getAttribute('data-mirror'));
        textField.addEventListener('input', (event) => {
            // avgColor(c, c) is a hack to canonicalize "#fff" as "#ffffff"
            colorSel.value = avgColor(event.target.value, event.target.value);
            calc.recalculate();
        });
        colorSel.addEventListener('input', (event) => {
            textField.value = event.target.value;
            calc.recalculate();
        });
        textField.value = colorSel.value;
        return textField;
    }
    
    recalculate() {
        let avg = avgColor(this.colorText1.value, this.colorText2.value);
        if (avg == null) {
            this.colorOut.value = '';
            this.textOut.value = 'Invalid input colour';
        } else {
            this.colorOut.value = this.textOut.value = avg;
        }
    }
}

new AvgColorCalculator(
    document.getElementById('colorSelector1'),
    document.getElementById('colorSelector2'),
    document.getElementById('avgColorSelector')
);
<table>
  <tr>
    <th><label>Input colour 1</label></th>
    <td><input type="color" id="colorSelector1" data-mirror="colorText1" value="#000000"><input type="text" id="colorText1" data-mirror="colorSelector1"></td>
  </div>
  <tr>
    <th><label>Input colour 2</label></th>
    <td><input type="color" id="colorSelector2" data-mirror="colorText2" value="#ffffff"><input type="text" id="colorText2" data-mirror="colorSelector2"></td>
  </tr>
  <tr>
    <th><label>Average colour</label></th>
    <td><input type="color" id="avgColorSelector" data-mirror="avgColorText" disabled><input type="text" id="avgColorText" data-mirror="colorSelector2" disabled></td>
  </tr>
</table>

  • Some exceptuonally good methods posted here, i cant help but feel slightly deflated that i wont be getting a call back to proceed further with the interview. None the less i shall be researching into shortened function writing as i have seen the arrow method but never fully understood it – DataCure Sep 14 at 8:17
  • I have just upvoted all answeres as there all pretty good responses, in terms of you wondering why "average colour:" was defined in css rather thsn the html, it is so that the words "average colour:" dont need to be added all the time the label is updated as the use of innerHTML would either clear the label using a = sign or keep appending to it using +=, whilst you can add them to the JS, i chose the css for something different – DataCure Sep 14 at 8:31
  • 1
    My advice is that if you're asked to write code to demonstrate something, then focus on making that code very good, and keep everything else simple. – 200_success Sep 14 at 8:35

CSS hex values are logarithmic.

Almost all 24 bit RGB data represents the square roots of the actual displayed intensity, this allows for a high dynamic range without a large memory overhead. When you double a color channel's value you cause the display to output 4 times as much light.

Calculating the mean without taking this into account results in a color that is too dark.

Incorrect and correct means

The mean of red and green should be a yellow not brown

span { display: list-item;}
<code>
<p>
<div>Incorrect (0xFF0000 + 0x00FF00) / 2 = 0x808000</div>
<span style="background:#FF0000">#FF0000</span>
<span style="background:#808000">#808000</span>
<span style="background:#00FF00">#00FF00</span>



<div>Correct ((0xFF0000 ** 2 + 0x00FF00 ** 2) / 2) ** 0.5 = 0xB4B400</div>
<span style="background:#FF0000">#FF0000</span>
<span style="background:#B4B400">#B4B400</span>
<span style="background:#00FF00">#00FF00</span>
</p>


</code>

To calculate the mean you must first square the color channels, then calculate the mean for each channel, then convert back to RGB via the square root.

Calculating the mean

r = Math.round(((A.r ** 2 + B.r ** 2) / 2) ** 0.5);
g = Math.round(((A.g ** 2 + B.g ** 2) / 2) ** 0.5);
b = Math.round(((A.b ** 2 + B.b ** 2) / 2) ** 0.5);
  • 1
    I think that it would be worthwhile to ask the interviewer or recruiter for clarification about the specifications. However, given the way that the question is posed, I believe that taking the linear midpoint in RGB color space is a reasonable assumption. (If you really want to, then using CIELAB color space might be even more "accurate".) – 200_success Sep 14 at 3:26
  • I doubt a recruiter or even most companies doing the hiring would understand the subtleties of linear vs. gamma-corrected RGB, unfortunately. You'd probably lose points for knowing more than them! – user1118321 Sep 14 at 3:49
  • @user1118321 Yes most don't know and that is why so many apps (including browsers) have such bad rendering. Once you know how this simple oversight effects rendering you will see it everywhere. youtube.com/watch?v=LKnqECcg6Gw – Blindman67 Sep 14 at 4:21
  • Exceptionally good answer and solution, but i agree with others on this one, it might not have been what they were seeking as they may have just been looking to proove your ability to convert hexideximal values into decimal and then calculate the average of the two given you cannot add the letters together from hex values to then divide by 2. I will try out your method as a learning curb, thanks for sharing – DataCure Sep 14 at 8:23

Hi All - Code Updated

I have seen some pretty good feedback, whilst I wasnt entirely satisfied with my first draft (which was already submitted to the employer). I decided to re-write my code yesterday.

I have also read some of your feedback and also applied it to my updated code, you will notice I have not copied anything you guys have posted, but rather seen your methods and tried to apply it to my new script.

It gets rid of the overcomplex javascript objects, which even I felt was overkill in an attempt to place some JS OOP in my example

var arrColours = [], arrColourHexRGB = [], arrDecColourRGB = [];
var elOutput = ""; strAVGColour = "";

function funUpdateColours(blColourSelector = true){
	const arrColours = [];
	const elColourSelector = document.getElementsByName("colourSelector");
	const elColourTxt = document.getElementsByName("colourText");
	const elOutput = document.getElementById("avgSelector");
	const elOutputTxt = document.getElementById("avgText");
	for (i = 0; i < elColourTxt.length; i++){
		if (blColourSelector){
			elColourTxt[i].value = elColourSelector[i].value;
		}else{
			elColourSelector[i].value = elColourTxt[i].value;
		}
		arrColours[i] = elColourTxt[i].value.replace("#", "");
	}
	arrColours.forEach((item, index) =>	arrColourHexRGB[index] = item.match(/.{1,2}/g));
	arrColourHexRGB.forEach((item, index, array) => arrDecColourRGB[index]= funHexToDec(item, index));
	const strAVGColour = funGetAverageColour (arrDecColourRGB);
	elOutput.value = strAVGColour;
	elOutputTxt.value = strAVGColour;
}

function funHexToDec(arrRGB, index){
	let arrRGBFuncOutput = [];
	for (z = 0; z < 3; z++){
		arrRGBFuncOutput[z] = parseInt(arrRGB[z], 16);
	}
	return arrRGBFuncOutput;
}

function funGetAverageColour(arrDecimalColours){
	let intR = 0, intG = 0, intB = 0;
	let strReturnValue = "";
	arrDecimalColours.forEach(function (item, index){
		intR = intR + item[0];
		intG = intG + item[1];
		intB = intB + item[2];
	});	
	intR = Math.ceil(intR / arrDecColourRGB.length);
	intR = funCheckHexOutput(intR);
	intG = Math.ceil(intG / arrDecColourRGB.length);
	intG = funCheckHexOutput(intG);
	intB = Math.ceil(intB / arrDecColourRGB.length);
	intB = funCheckHexOutput(intB);
	return strReturnValue = "#" + intR + intG + intB;
	
}
function funCheckHexOutput(strHex){
	strHex = strHex.toString(16);
	if (strHex.length < 2){
		strHex = "0" + strHex;
	}
	return strHex;
}
body{
	font-family: arial, helvetica;	
	margin: 0 auto
}
header{
	background: #000;
	color: #c0c0c0;
	font: 14pt arial;
	height: 2em;
	padding: 0.5em 0 0 0.5em;
	width: 100%
}
body{
	background: #444
}
input[type="text"]{
	width: 120px;
}
main{
	border-radius:10px;
	background: #AAA;
	left: 100px;
	padding: 30px;
	position: absolute;
	top: 25%;
	width: 360px
}
div#divOutputColour{
	background: #808080;
	border: 1px solid black;
	height: calc(100% - 60px);
	position: absolute;
	right: 30px;
	top: 30px;
	width: 100px
}
label#elAvgColourTxt::before{
	content: 'Average Colour: ';
}
span#spanOutputText{
	width: 100%
}
<!Doctype html>
<html>
	<head>
		<title>Average Colour Selector</title>
		<script src="js/colourSelector.js"></script>
		<link rel="stylesheet" href="css/colourSelector.css">
		<meta charset="UTF-8">
	</head>
	<body>
		<header>Return Mean Hex Colour Code</header>
		<main>
			<label>Colour 1:</label><input name="colourSelector" type="color" value="#FFFFFF" onchange="funUpdateColours(true)"><input type=text value="#FFFFFF" name="colourText" onchange="funUpdateColours(false)" maxlength=7><br>
			<label>Colour 2:</label><input name="colourSelector" type="color" value="#000000" onchange="funUpdateColours(true)"><input type=text value="#000000" name="colourText" onchange="funUpdateColours(false)" maxlength=7><br>
			<label>Colour 3:</label><input name="colourSelector" type="color" value="#000000" onchange="funUpdateColours(true)"><input type=text value="#000000" name="colourText" onchange="funUpdateColours(false)" maxlength=7><br>
			<label>Colour 4:</label><input name="colourSelector" type="color" value="#000000" onchange="funUpdateColours(true)"><input type=text value="#000000" name="colourText" onchange="funUpdateColours(false)" maxlength=7><br><br>
			<label>Average:</label><input id="avgSelector" type="color" value="#808080" ><input type=text value="#808080" id="avgText" disabled>
		</main>
		
	</body>
</html>

I was going to do a padding in that it prefixed "0" regardless and performed a substring that returned the first 2 characters from the right

Example in other languages such as ASP / VB right("0" + intB, 2) which would cut out the need for an if statement or inline if statement as the the values would become

2 digit Hex example: A9 becomes 0A9 = reading from right to left 2 chars = A9 1 digit Hex example: 5 becomes = 05 = reading from right to left 2 chars = 05

Changes

  • Removed the Object
  • Removed the Error handling as re-wrote script
  • Redeveloped the HTML to allow for array of elements.

Feedback I adhered to

  • Added constant declarations

  • Used arrow functions

  • Padded the Hex values (valid bug I had not spotted during testing)

  • Used getElementById (and getElementByName) instead of querySelector

Disregarded feedback

  • Span Tag

    Whilst the close span tag was valid feedback, it was an oversight due to last minute changes, I should of proof read my code before submitting it.

  • Lineal Average

    Whilst I was impressed by this technique, it wasnt clear from the task spec that was what the recruiter wanted, I have however made a note, to trial the example out as I think its good knowledge to have for in the future.

  • EventListner

    There was a suggestion for adding event handler for the on change over HTML triggered onchange event, whilst I appreciate where the suggestion was coming from, I was skeptical to use this, whilst it would of been fine for the demo / task, in general it would be bad practice (in my option) to run a function on every element change the webpage may undergo, which would risk the employer coming to the wrong conclusion that this is a method id adopt in general.

  • Naming conventions

    My previous employer was keen that each variable is prefixed variable type abreviation in order to make the code more user friendly to read. fun for function, _st_r for string etc. I have to agree with my previous employer - it is handy to have. This includes detailing what the function does e.g.: funReturnHex gives a clear indication of what the function is going to do, even before you read the code or the developer has a clear understanding of what the function should do in the event of debugging code in the future.

Stuff I will add to my code later on

  • Tickbox to choose lineal average

  • The example shown on how to convert short HEX to full length (FFF to FFFFFF)

I didnt have chance to fully study what it was doing, whilst I have a vague understanding, i didnt want to add it into my code until I have fully understood it, as I am not a fan of copy and pasting other peoples code, but rather prefer to understand what im implementing

Your Answer

 

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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