Changing background with JavaScript

Question

I am in the process of learning JS and I constantly have feeling that my code is bloat and not optimal.

I built this simple DEMO of background changing on click. Can somebody tell me if this is good approach to build this, what can be improved? Can I write this with less code?

jsFiddle

HTML

<div id="one">
    <div id="two"></div>
</div>

CSS

div {
width: 200px;
height: 200px;
border: 1px solid dimgrey;
display: flex;
margin: auto;
transition: background-color .4s ease-out;
}

div > div {
width: 100px;
height: 100px;
border: 1px solid darkgray;
}

JS

var divOne = document.getElementById("one"),
    divTwo = document.getElementById("two"),
    divOneStyle = divOne.style,
    divTwoStyle = divTwo.style,
    RGB = ["red","green","blue"],
    YPB = ["yellow","purple","black"];




var colorMeRGB = function (event) {
    divOneStyle.backgroundColor = RGB[Math.floor(Math.random() * RGB.length)];
    event.stopPropagation();
};
divOne.addEventListener("click",colorMeRGB,false);

var colorMeYPB = function (event) {
    divTwoStyle.backgroundColor = YPB[Math.floor(Math.random() * YPB.length)];
    event.stopPropagation();
};
divTwo.addEventListener("click",colorMeYPB,false);

Answer 1

You could create a curried function that takes an array of colors, an element, and an event. Then you build your other functions by partial application:

var divOne = document.getElementById("one"),
  divTwo = document.getElementById("two"),
  RGB = ["red","green","blue"],
  YPB = ["yellow","purple","black"];

var colorMe = function(colors) {
  return function(elem) {
    return function(event) {
      elem.style.backgroundColor = colors[Math.random() * colors.length |0];
      event.stopPropagation();
    };
  };
}

var colorMeRGB = colorMe(RGB);
var colorMeYPB = colorMe(YPB);

divOne.addEventListener("click", colorMeRGB(divOne), false);
divTwo.addEventListener("click", colorMeYPB(divTwo), false);

In you have something like LoDash at hand, you can use curry:

var colorMe = _.curry(function(colors, elem, event) {
  ...
})

You could also use this inside the event, which refers to the element, but find it can be confusing, and you may lose flexibility.

If you think of data flow and transformations, rather than steps, you can come up with a slightly more DRY solution:

var colorMe = _.curry(function(colors, elem, event) {
  elem.style.backgroundColor = colors[Math.random() * colors.length | 0];
  event.stopPropagation();
});

var data = [
  {id: 'one', colors: ['red','green','blue']},
  {id: 'two', colors: ['yellow','purple','black']}
];

data.forEach(function(x) {
  var elem = document.getElementById(x.id);
  elem.addEventListener('click', colorMe(x.colors, elem), false);
});

But if you're reusing the elements it is recommended to cache them in variables.

Note I used |0 to floor the number. This is quite common, and it cleans up that line a bit.

DEMO: http://jsfiddle.net/uqkutqht/1/